Skip to content

Fix incorrect TVOC / NOx values when network option is cellular #303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 15, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions examples/OneOpenAir/OneOpenAir.ino
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ enum NetworkOption {
};
NetworkOption networkOption;
TaskHandle_t handleNetworkTask = NULL;
static bool otaInProgress = false;
static bool firmwareUpdateInProgress = false;

static uint32_t factoryBtnPressTime = 0;
static AgFirmwareMode fwMode = FW_MODE_I_9PSL;
Expand Down Expand Up @@ -318,8 +318,8 @@ void loop() {
// Schedule to feed external watchdog
watchdogFeedSchedule.run();

if (otaInProgress) {
// OTA currently in progress, temporarily disable running sensor schedules
if (firmwareUpdateInProgress) {
// Firmare update currently in progress, temporarily disable running sensor schedules
delay(10000);
return;
}
Expand Down Expand Up @@ -562,38 +562,33 @@ void checkForFirmwareUpdate(void) {
agOta = new AirgradientOTACellular(cellularCard);
}

// Indicate main task that ota is performing
Serial.println("Check for firmware update, disabling main task");
otaInProgress = true;
if (configuration.hasSensorSGP && networkOption == UseCellular) {
// Only for cellular because it can disturb i2c line
Serial.println("Disable SGP41 task for cellular OTA");
ag->sgp41.end();
}
// Indicate main task that firmware update is in progress
firmwareUpdateInProgress = true;

agOta->setHandlerCallback(otaHandlerCallback);
agOta->updateIfAvailable(ag->deviceId().c_str(), GIT_VERSION);

// Only goes to this line if OTA is not success
// Only goes to this line if firmware update is not success
// Handled by otaHandlerCallback

otaInProgress = false;
if (configuration.hasSensorSGP && networkOption == UseCellular) {
// Re-start SGP41 task
if (!sgp41Init()) {
Serial.println("Failed re-start SGP41 task");
}
}
// Indicate main task that firmware update finish
firmwareUpdateInProgress = false;

delete agOta;
Serial.println();
}

void otaHandlerCallback(AirgradientOTA::OtaResult result, const char *msg) {
switch (result) {
case AirgradientOTA::Starting:
case AirgradientOTA::Starting: {
Serial.println("Firmware update starting...");
if (configuration.hasSensorSGP && networkOption == UseCellular) {
// Temporary pause SGP41 task while cellular firmware update is in progress
ag->sgp41.pauseHandle();
}
displayExecuteOta(result, fwNewVersion, 0);
break;
}
case AirgradientOTA::InProgress:
Serial.printf("OTA progress: %s\n", msg);
displayExecuteOta(result, "", std::stoi(msg));
Expand All @@ -602,6 +597,10 @@ void otaHandlerCallback(AirgradientOTA::OtaResult result, const char *msg) {
case AirgradientOTA::Skipped:
case AirgradientOTA::AlreadyUpToDate:
displayExecuteOta(result, "", 0);
if (configuration.hasSensorSGP && networkOption == UseCellular) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the idea was to not interrupt the polling if no firmware update is needed. So I'm a bit surprised that we need to resume here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, even if it has no impact because it haven't paused yet. Fixed it.

e7603a7#diff-a9eec5b0808e2ecb2731d6bcffbc2fd06fefc1b8643ff524865db674caf59fb5R597

// Cellular firmware update finish, resuming SGP41 task
ag->sgp41.resumeHandle();
}
break;
case AirgradientOTA::Success:
displayExecuteOta(result, "", 0);
Expand Down Expand Up @@ -665,7 +664,11 @@ static void displayExecuteOta(AirgradientOTA::OtaResult result, String msg, int
}
delay(1000);
}
oledDisplay.setAirGradient(0);

if (ag->isOne()) {
oledDisplay.setAirGradient(0);
oledDisplay.setBrightness(0);
}
break;
}
default:
Expand Down Expand Up @@ -1548,10 +1551,7 @@ void restartIfCeClientIssueOverTwoHours() {

void networkingTask(void *args) {
// OTA check on boot
#ifdef ESP8266
// ota not supported
#else
// because cellular it takes too long, watchdog triggered
#ifndef ESP8266
checkForFirmwareUpdate();
checkForUpdateSchedule.update();
#endif
Expand Down
21 changes: 21 additions & 0 deletions src/Sgp41/Sgp41.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,22 @@ void Sgp41::handle(void) {
}

#else

void Sgp41::pauseHandle() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propose to rename to simply "pause()" and "resume()"

onPause = true;
Serial.println("Pausing SGP41 handler task");
// Set latest value to invalid
tvocRaw = utils::getInvalidVOC();
tvoc = utils::getInvalidVOC();
noxRaw = utils::getInvalidNOx();
nox = utils::getInvalidNOx();
}

void Sgp41::resumeHandle() {
onPause = false;
Serial.println("Resume SGP41 handler task");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we used the -ing form in pause(), I propose to also use it here

-> Resuming SGP41 ...

}

/**
* @brief Handle the sensor conditioning and run time udpate value, This method
* must not call, it's called on private task
Expand All @@ -152,6 +168,11 @@ void Sgp41::_handle(void) {
uint16_t srawVoc, srawNox;
for (;;) {
vTaskDelay(pdMS_TO_TICKS(1000));

if (onPause) {
continue;
}

if (getRawSignal(srawVoc, srawNox)) {
tvocRaw = srawVoc;
noxRaw = srawNox;
Expand Down
5 changes: 5 additions & 0 deletions src/Sgp41/Sgp41.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ class Sgp41 {
bool begin(TwoWire &wire, Stream &stream);
void handle(void);
#else
/* pause _handle task to read sensor */
void pauseHandle();
/* resume _handle task to read sensor */
void resumeHandle();
void _handle(void);
#endif
void end(void);
Expand All @@ -32,6 +36,7 @@ class Sgp41 {
int getTvocLearningOffset(void);

private:
bool onPause = false;
bool onConditioning = true;
bool ready = false;
bool _isBegin = false;
Expand Down