Bugfix/sen5x voc state

[oscgonfer]

Fixes #87

[pylonsoft]

I'm quite convinced this version won't work. The format of the state bytes are the same for reading and writing, so you have to insert the CRC bytes again when you send the state to the sensor. The previous version where you stored the CRC bytes together with the state probably worked as the state was sent in exactly the same format as it was received.

For me the saving of the VOC state isn't that important because I have a Kit 2.3 which comes with a battery so the sensor won't ever be without power (if SEN55 sensors are activated of course, so the software is in continuous mode), the VOC state doesn't have to be saved.

[oscgonfer]

I'm quite convinced this version won't work. The format of the state bytes are the same for reading and writing, so you have to insert the CRC bytes again when you send the state to the sensor. The previous version where you stored the CRC bytes together with the state probably worked as the state was sent in exactly the same format as it was received.

Issue is (I think) that when sending the VOCState buffer back, the CRC is re-added on the sen_sendCommand function:

smartcitizen-kit-2x/sam/src/SckUrban.cpp

Lines 2476 to 2493 in 803d3f2

	// Add space for CRC bytes (one every two bytes)
	if (byteNumber > 0) bufferSize += byteNumber + (byteNumber / 2);
	uint8_t toSend[bufferSize];
	uint8_t i = 0;
	toSend[i++] = static_cast<uint8_t>((wichCommand & 0xFF00) >> 8);
	toSend[i++] = static_cast<uint8_t>((wichCommand & 0x00FF) >> 0);
	// Prepare buffer with CRC every third byte
	uint8_t bi = 0;
	if (byteNumber > 0) {
	while (bi < byteNumber) {
	toSend[i++] = buffer[bi++];
	toSend[i++] = buffer[bi++];
	uint8_t calcCRC = sen_CRC(&buffer[bi - 2]);
	toSend[i++] = calcCRC;
	}
	}

Which leads me to think that what we are sending back to the VOC state is wrong, hence this PR.

For me the saving of the VOC state isn't that important because I have a Kit 2.3 which comes with a battery so the sensor won't ever be without power (if SEN55 sensors are activated of course, so the software is in continuous mode), the VOC state doesn't have to be saved.

Sure. Just keep in mind that the device however resets every night (in CET) for a sanity reset. This can be disabled in the shell. Ideally, the VOC state is saved before every reset, but I am not fully confident this works at the moment.

[pylonsoft]

You're right, I'm wrong. I didn't consider the sen_sendCommand code. I still didn't got a complete view of the code and it's inner workings. I'd probably make that reciprocal, so sen_receiveBuffer will remove the CRC bytes as sen_sendCommand adds them. I know is quite a refactoring but having different behavior on the two sides of the communication isn't good for maintenance.

I uploaded the code with this branch applied and wait to see how it performs.

[pylonsoft]

I guess the call to vocStateFromSensor() in idle() should be eliminated because if the VOC sensor is active we're always in continuous mode so that call is never reached. If VOC sensor isn't active we don't need to save the state.

[oscgonfer]

I think we are now onto something (see data from comparison here: https://api.smartcitizen.me/ui/experiments/29/)

The importance to recover well the voc index state can be seen at the midnight reset. I'll set two devices with bugfix HEAD without the midnight reset to check.

[oscgonfer]

I guess the call to vocStateFromSensor() in idle() should be eliminated because if the VOC sensor is active we're always in continuous mode so that call is never reached. If VOC sensor isn't active we don't need to save the state.

That's now the tricky part. Either we get it periodically (still to figure out how), or on reset / power off (properly).

[pylonsoft]

I add the last day of my two sensors:

I guess by midnight reset you mean the sanity reset at 03:00 UTC, in my case it's 05:00h (UTC+2h) and you can see it for the yellow graph which is my development (currently in-house) system. The green graph is my outdoor system which runs version 0.9.10 with my changes applied (it's installed at a height of 4 meters I don't do updates that often). As you can see, the VOC index value does change less at sanity reset although it's noticeable. I guess that the sensor must run for at least 12 hours uninterrupted to get a result in the VOC state worth to be stored. Storing the values in the wrong format seems to be better than nothing but doesn't help to get a correct result.

[pylonsoft]

Either we get it periodically (still to figure out how), or on reset / power off (properly).

Periodically isn't that hard, just save the time of the last save and check the time difference at every update(). But checking for very special configurations (p.e. forcedContinousMode) may be hard because we have to wait that time after starting measurement again.
I'd vote for doing that at reset time, then is doesn't matter leaving measurement mode. But I guess you should remove the 10 minutes check when restoring it otherwise you don't have to save it when powering off I think (given I interpret the reason of a user to power off the device correctly).

[pylonsoft]

I was wrong again, we don't have to leave measurement mode to read the VOC state from the sensor, that seems to be necessary only for writing it back, although I couldn't find the reason for that in the datasheet.
But anyway I would save the state at reset and power off and read it immediately before saving it to EEPROM.

[oscgonfer]

Hey, so the approach currently implemented on the latest commit (8dce8f3) is more or less as you mention, but with some slight differences:

• Turn-on device and read EEPROM state
• Feed that to sensor (we only feed it if not too old and valid)
• Start readings
• At every reading, get the vocState from the sensor and keep it in runtime - store the timestamp at which that was read and the validity of the state (simple check of all zeros or not, at least for now)
• If reset happens, store the latest vocState into EEPROM, along with valid flag and time.

The particularity is the "incorrect shutdown" process, which would lose the latest vocState. This is a downside but it doesnt matter so much. We could have the vocstate stored into EEPROM after every read, but we have limited read/writes, and we don't want to kill it.

[pylonsoft]

Feed that to sensor (we only feed it if not too old and valid)

Why does it have to be not too old? If a user switches the device off for the night for example, why shouldn't we use the saved VOC state once the device is turned on in the morning?

At every reading, get the vocState from the sensor and keep it in runtime - store the timestamp at which that was read and the validity of the state (simple check of all zeros or not, at least for now)

Why is it read that often? Why not reading it just before we reset/go to sleep?

The particularity is the "incorrect shutdown" process, which would lose the latest vocState. This could be stored into EEPROM, but we have limited read/writes, and we don't want to kill it.

Could you give more information on that? Do you mean the shutoff by long pressing the button? EEPROM has limited writes but that number is very hight (>100'000). Why do you think the device will be turned off that often?
Remember, we don't need to store the VOC state if not in continuous mode if that "incorrect shutdown" is meant.

[oscgonfer]

Why does it have to be not too old? If a user switches the device off for the night for example, why shouldn't we use the saved VOC state once the device is turned on in the morning?

Not fully sure where the exact time is, but in the datasheet says "short".

Why is it read that often? Why not reading it just before we reset/go to sleep?

Because controlling the sensor before the reset may find the sensor in an unknown state (could be for instance OFF) and by doing it on the update we guarantee is operational. At least that's how I see it. It doesn't cost much time to read the VOC state.

Could you give more information on that? Do you mean the shutoff by long pressing the button? EEPROM has limited writes but that number is very hight (>100'000). Why do you think the device will be turned off that often? Remember, we don't need to store the VOC state if not in continuous mode if that "incorrect shutdown" is meant.

By incorrect shutdown process I mean switching the device off by removing power and not giving it time to safely store things before shutting down (Edit: this is fine, no need to cover for that case). And by the EEPROM writes limitation: if we read the state every time we hit update, and we store it in EEPROM, that gives us roughly a year of EEPROM r/ws. However, now we read it every time, and we store it only on reset/power-off.

[pylonsoft]

Not fully sure where the exact time is, but in the datasheet says "short".

"Short" isn't a defined time period. Given that the sensor needs 12 hours of continuous operation to even establish a VOC state I would say that every time frame that is less than that is "short".
If that time frame is 10 minutes a user that doesn't run the device 24 hours a day won't ever get suitable VOC index values.

Because controlling the sensor before the reset may find the sensor in an unknown state (could be for instance OFF) and by doing it on the update we guarantee is operational.

It cannot be in off state because the VOC sensor requires continuous mode and the storage of the VOC state may be and probably should be coupled to that mode.

By incorrect shutdown process I mean switching the device off by removing power and not giving it time to safely store things before shutting down.

I don't understand why you insist on saving the VOC state even when the VOC sensor is inactive. If you have to handle the case where the sensor is turned on and off you add complexity without a need. The VOC state is only needed if the VOC index is used and in that case the continuous mode is used where the device will not be turned off.
So instead of checking for the SEN55 model (which isn't correct anyway as the SEN54 model has the VOC sensor too) I would check for continuous mode.

[oscgonfer]

"Short" isn't a defined time period. Given that the sensor needs 12 hours of continuous operation to even establish a VOC state I would say that every time frame that is less than that is "short". If that time frame is 10 minutes a user that doesn't run the device 24 hours a day won't ever get suitable VOC index values.

That's incorrect. Sensor requires roughly 1h to give good Voc state. Actually, non-zero VOC states (who knows if valid or not) are returned after very few minutes of sensor operation. See the issue in the manufacturer repository I opened a while back and their answer

It cannot be in off state because the VOC sensor requires continuous mode and the storage of the VOC state may be and probably should be coupled to that mode.

Yes it can. You can disable the sensor manually, and ultimately the idea is to save battery by not having the sensor constantly powered.

I don't understand why you insist on saving the VOC state even when the VOC sensor is inactive. If you have to handle the case where the sensor is turned on and off you add complexity without a need. The VOC state is only needed if the VOC index is used and in that case the continuous mode is used where the device will not be turned off. So instead of checking for the SEN55 model (which isn't correct anyway as the SEN54 model has the VOC sensor too) I would check for continuous mode.

I don't understand the "insist on saving the VOC state even if the sensor is inactive". The voc state will be queried, and then saved if it returns a valid response. There's no need to handle anything complex, I don't know where that comes from.

As per the Sen54-55 you are right. For whatever reason I got that mixed up.

[pylonsoft]

You can disable the sensor manually, and ultimately the idea is to save battery by not having the sensor constantly powered.

Sure you can disable the sensor but in that case you don't need the VOC state to be saved over resets/power offs. If the user is not interested in the VOC index value (sensor disabled) it doesn't matter what value is read from the sensor and if that value makes sense.

The voc state will be queried, and then saved if it returns a valid response.

I understood that but as mentioned above I see no need to save the state if the user don't need the VOC index value.
Anyway, you're right that reading the state doesn't cost much time and saving it at reset/power is almost free, so why not doing it.
I noted the complexity because if you don't care for the VOC state if the VOC index value isn't requested by the user you can do the read out at reset time only which makes much more sense for a user trying to understand the code. Reading it in every cycle just because we can isn't what I would expect. That about the same as having a display for the motor oil temperature in an electric car.

[oscgonfer]

Further work on this:

1. a firmware flag now allows the change between measurement mode (PM and everything) and rht/gas only mode: allowRHTGasMode in SCKUrban.h: https://github.com/fablabbcn/smartcitizen-kit-2x/blob/bugfix/sen5x-voc-state/sam/src/SckUrban.h#L472
2. The VOCState buffer bug was not a bug. It was all good from the beginning. Now this PR actually has solved related issues to the VOC sensor, but the CRC assumption was mistaken. However, there was some issues with the length of the buffers request.

On the bright side of things, the allowRHTGasMode has a great impact on the temperature and battery duration of the kit.

Battery Enhanced by a very long-shot

VOC index (still one value is 0 after recovering, but it comes back to a "reasonable" spot - no way to judge though)

Temperature -1.5degC difference in overheating