Hi, obviously a lot of work gone into NodeManager thanks, I'm quite new to MySensors but have started working on.a few sensors around my home.

I was just looking through the code (NodeManager.cpp) and an apparent issue jumped out at me (I write C++ for a living on very large boxes but still worry about performance!)
The two functions NodeManager::_saveConfig and NodeManager::_loadConfig are used to save the _sleep_time value to EEPROM. The code splits the long value into 3 parts by dividing by successively 255 . On loading the value is reconstructed by multiplying by 255.
I'm pretty sure that you meant to use 256 to split the long value into three independent bytes to store.
Dividing or multiplying by 255 is potentially expensive whereas dividing or multiplying by 256 is much faster simply requiring shifts.
The normal code for splitting a long value into individual bytes would be something like
uint8_t byte0 = _sleep_time & 0xff; // compiler should generate a simple byte load
uint8_t byte1 = (_sleep_time >> 8 ) & 0xff; // compiler should still generate a simple byte load from the second byte of _sleep_time
uint8_t byte2 = (_sleep_time >> 16) & 0xff; // as before should still be a byte load.
I'm not sure how good the gcc optimiser is for the AVR code but, putting the shift&mask operations directly into the calls of saveSave(...) may avoid having to allocate local variables as in
// save the 3 bits
saveState(EEPROM_SLEEP_SAVED,1);
saveState(EEPROM_SLEEP_1,_sleep_time & 0xff);
saveState(EEPROM_SLEEP_2,(_sleep_time >> 8 ) & 0xff);
saveState(EEPROM_SLEEP_3, (_sleep_time >> 16) & 0xff);

I am not an expert in in the AVR architecture or instruction set so maybe there is a good reason why you are using 255 instead of 256 - it just seems strange!

Regards

Graham

@user2684 I have looked at the changes in https://github.com/mysensors/NodeManager/pull/174 which incorporates an alternative improved implementation for the _LoadConfig/_SaveConfig functions (interesting that using an intermediate union requires less code than my shifting version- presumably due to differences in the way the GCC optimiser works for an AVR target).
I spotted a coupld of minor points and was also able to further reduce the code size.
Firstly as we are writing C++ rather than C the declaration of the union should just be
// Local union used to split _sleep_time into bytes
union tLongByteArrayCombo{
long long_value;
uint8_t byte_array[4];
} ;
(The typedef systax, although valid C++ is unnecessary - a simple union declaration is sufficient.)
There is also a typo - the original had a member called byte_arrray instead of byte_array.

You can also save 24 bytes of code by avoiding the use of a local union variable and simply aliasing _sleep_time as the union type. So instead of _load_config doing -
tLongByteArrayCombo c;
c.byte_array[0] = loadState(EEPROM_SLEEP_1);
c.byte_array[1] = loadState(EEPROM_SLEEP_2);
c.byte_array[2] = loadState(EEPROM_SLEEP_3);
c.byte_array[3] = 0;
_sleep_time = c.long_value;
You can do
(((tLongByteArrayCombo&)_sleep_time).byte_array )[0] = loadState(EEPROM_SLEEP_1);
(((tLongByteArrayCombo&)_sleep_time).byte_array )[1] = loadState(EEPROM_SLEEP_2);
(((tLongByteArrayCombo&)_sleep_time).byte_array )[2] = loadState(EEPROM_SLEEP_3);
(((tLongByteArrayCombo&)_sleep_time).byte_array )[3] = 0;
And in _save_config instead of
tLongByteArrayCombo c;
c.long_value = _sleep_time;
saveState(EEPROM_SLEEP_1, c.byte_array[0]);
saveState(EEPROM_SLEEP_2, c.byte_array[1]);
saveState(EEPROM_SLEEP_3, c.byte_array[2]);
you can use
saveState(EEPROM_SLEEP_1, (((tLongByteArrayCombo&)_sleep_time).byte_array )[0] );
saveState(EEPROM_SLEEP_2, (((tLongByteArrayCombo&)_sleep_time).byte_array )[1] );
saveState(EEPROM_SLEEP_3, (((tLongByteArrayCombo&)_sleep_time).byte_array )[2] );

In my specific sample sketch I had the following code sizes

Original NodeManager.cpp 28242 Bytes
NodeManager with patch from pull rq 174 - 28154 Bytes (saving of 88 Bytes of code(
Patched NodeManager with additional changes above - 28130 Bytes ( saving another 24 Bytes of code)

Hi, obviously a lot of work gone into NodeManager thanks, I'm quite new to MySensors but have started working on.a few sensors around my home.

I was just looking through the code (NodeManager.cpp) and an apparent issue jumped out at me (I write C++ for a living on very large boxes but still worry about performance!)
The two functions NodeManager::_saveConfig and NodeManager::_loadConfig are used to save the _sleep_time value to EEPROM. The code splits the long value into 3 parts by dividing by successively 255 . On loading the value is reconstructed by multiplying by 255.
I'm pretty sure that you meant to use 256 to split the long value into three independent bytes to store.
Dividing or multiplying by 255 is potentially expensive whereas dividing or multiplying by 256 is much faster simply requiring shifts.
The normal code for splitting a long value into individual bytes would be something like
uint8_t byte0 = _sleep_time & 0xff; // compiler should generate a simple byte load
uint8_t byte1 = (_sleep_time >> 8 ) & 0xff; // compiler should still generate a simple byte load from the second byte of _sleep_time
uint8_t byte2 = (_sleep_time >> 16) & 0xff; // as before should still be a byte load.
I'm not sure how good the gcc optimiser is for the AVR code but, putting the shift&mask operations directly into the calls of saveSave(...) may avoid having to allocate local variables as in
// save the 3 bits
saveState(EEPROM_SLEEP_SAVED,1);
saveState(EEPROM_SLEEP_1,_sleep_time & 0xff);
saveState(EEPROM_SLEEP_2,(_sleep_time >> 8 ) & 0xff);
saveState(EEPROM_SLEEP_3, (_sleep_time >> 16) & 0xff);

I am not an expert in in the AVR architecture or instruction set so maybe there is a good reason why you are using 255 instead of 256 - it just seems strange!

Regards

Graham