Changes for saving childs value in EEPROM

[j54n1n]

Hello there,

In search for issue #542 I noticed that saving a value of a sensor in the EEPROM of the microcontroller allows only a limited range for positive numbers up to 9999 and has similar issues with the decimal part of floating point numbers.

I have seen that the code uses following layout: [TYPE][SIGN][INT_PART1][INT_PART2][DEC_PART1][DEC_PART2]

I would like to propose that the packing and unpacking could be done in a different manner using bitshifting and pointer arithmetic with casts, since either a uint16_t or a 32-bit float gets saved in the EEPROM. This would mean that the SIGN byte could be omitted since it is saved in the numerb itself and the entire frame could be shortened to 5 bytes instead of 6. Also this would allow to save the entire range of integer and float numbers in the EEPROM.

The result would like be something like this: For 16-bit ints [TYPE][INT16_PART1][INT16_PART2][UNUSED][UNUSED] and for floats [TYPE][FLOAT_PART1][FLOAT_PART2][FLOAT_PART3][FLOAT_PART3]

I will try to publish a pull request with the mentioned changes.

Kind regards.

[user2684]

Great idea, this was a known issue I never had the chance to look into :-) Just one question before merging it: what happens is a user upgrades NodeManager and was storing something in the EEPROM before? I guess it will get corrupted/wrong data right? Not a problem of course, I can always make this clear in the release notes when going GA with this version, but just to confirm my understanding is correct

[j54n1n]

Great idea, this was a known issue I never had the chance to look into :-) Just one question before merging it: what happens is a user upgrades NodeManager and was storing something in the EEPROM before? I guess it will get corrupted/wrong data right? Not a problem of course, I can always make this clear in the release notes when going GA with this version, but just to confirm my understanding is correct

Yes the binary format changes, so for the first time after an upgrade there will be false data restored for both integer and float values and for each successive child ID sensor values stored in EEPROM, except if the sensor value type does not match then nothing will be restored. So mabye only the first stored sensor child value in EEPROM will restore false data. But since storage for auto assigned child IDs will never use child ID 0 even the first value type of child ID 1 should be already misaligned and therefore not restored.

For example with the old layout with float values we have:	POS.	TYPE	SIGN	INT1	INT2	DEC1	DEC2
0x00	(ID 0, not used)	(not used)	(not used)	(not used)	(not used)	(not used)
0x06	V_RAIN (from ID 1)	0x00	0x00	0x02	0x04	0xFF
0x0C	V_WIND (from ID 2)	0x00	0x00	0x01	0x02	0xFF
0x12	V_DIRECTION (from ID 3)	0x00	0x03	0x3B	0x00	0xFF

compared to the new alignment this will change to:	POS.	TYPE	NUM1	NUM2	NUM3	NUM4	(cont.)
0x00	(ID 0, not used)	(not used)	(not used)	(not used)	(not used)	V_RAIN (from ID 1)
0x06	0xDE	0xAD	0xBE	0xEF	V_WIND (from ID 2)	0xDE
0x0C	0xAD	0xBE	0xEF	V_DIRECTION (from ID 3)	0xDE	0xAD
0x12	0xBE	0xEF	(not used)	(not used)	(not used)	(not used)

Following the new alignment the type field at position 0x24 will align again and collide with a previously stored value of child IDs 6 (old) and 7 (new), but if both child types are different it should also not be restored. Basically at L328 it will return when the TYPE does not match.

But in my case I tested it only with my weather station node and after an sensor data update it will be overwritten anyway with the new value and layout.

For sure a note should be included for the next release.

[user2684]

Thanks for the detailed explanation, makes sense to me! Definitely the advantage of using this approach overcomes the drawback of having a false read, once. I'll make it clear anyway of course in the release notes when this version will be officially released. Merging your PR into development right now. Thanks again for the effort!