Architecture Review

[user2684]

This PR reviews the architecture of NodeManager.

NodeManager files structure:

• New Arduino-compatible library structure to allow easier integration, more consistent upgrades across version, and a complete set of examples.
• The code of each sensor is now in a dedicated header file under the sensors directory which can be directly included by the user when needed. Implementation of the class is inline. Required libraries and OTA configuration request handling is now centralized inside the class. This approach should facilitate integration of new sensors (also because can be developed in parallel) and centralize all the sensor-related tasks
• Split NodeManager's core classes in individual files
• Moved v1.7 main sketch into examples/Template and examples from the README file into individual sketches in examples
• Added keywords.txt file with NodeManager's capabilities defined as LITERAL1, NodeManager classes and sensors as KEYWORD1 and NodeManager functions as KEYWORD2. Sensor specific functions are not supposed to be defined here
• Added Travis CI additional tests covering all the capabilities, the sensors, the examples and previous versions of the MySensors library

NodeManager core capabilities:

• No more the need to create a NodeManager instance in the sketch. An global object called nodeManager is now created as extern in within the library. This also simplify the code since there is no more the need to pass its reference throughout the code and save some more memory
• Deprecated the USE_* defines since now the user can import a sensor directly from the sketch, just before invoking the constructor (e.g. #include <sensors/SensorBattery.h>)
• Renamed FEATURE_* into NODEMANAGER_* (e.g. FEATURE_DEBUG is now NODEMANAGER_DEBUG). This is because being a library we need to have our variables somehow linked to it
• Added NODEMANAGER_OTA_CONFIGURATION configuration setting, OFF by default, when ON, enables OTA configuration. There is no more the need to create an instance of SensorConfiguration since done automatically within the code when NODEMANAGER_OTA_CONFIGURATION is ON
• Added NODEMANAGER_SERIAL_INPUT default to OFF; which allows reading from the serial port at the end of each loop cycle expecting command formatted according to the MySensors serial protocol. Useful for debugging interactive sensors such as relays without the need to send messages over the network (fixes Ability to provide messages through Serial input for debugging purposes #303)
• Added NODEMANAGER_DEBUG_VERBOSE, default to OFF, so to allow saving additional memory by moving heavy debug output like those in LOG:MSG into this mode
• When NODEMANAGER_POWER_MANAGER is ON, powerOn() and powerOff() are also called during NodeManager and Sensor setup() (fixes PowerManager should be turned on also during setup() #361)
• Added debug() macro which is using MySensors' hwDebugPrint() resulting in a better compatibility, more readable and concise code (fixes For debug output, use the same approach of MySensors library #323 NodeManager and MySensors openHAB binding: constant reset #373)
• Debug output is now fully compatible with the one used by the MySensors library (fixes Improve debug output #180). Documentation and regexp are provided below and the code has been already integrated into LogParser (https://www.mysensors.org/build/parser)
• Added support for OTA debug by leveraging MySensors library functionality and added commented defines in the sketch template (fixes Provide remote logging #132)
• sleepBetweenSend() call wait() and no more sleep() to avoid retries with Ack messages (fixes sleepBetweenSend causes ack retries #405)

NodeManager API:

• Added NodeManager::setTimezone() to set the hour offset to be applied for when received a UTC timestamp from the controller (fixes Add option to store the time in the local timezone #368)
• Added setSyncTimeOnSetup(), setSyncTimeAfterSleep() and setSyncTimeAfterInterval() to the NodeManager class to better control how often, if and when to sync the time with the controller, especially when the user wants to rely on a RTC (for this reason time with the RTC if configured is always synchronized regardless of these settings) (fixes RTC: add option to customize how often to sync with controller #353)

Sensor API:

• Added a Measure Timer so to allow splitting between taking measures and reporting. If not set explicitly, will be automatically set as the reporting timer so to align measures and reporting (fixes Split between reporting and taking a measurement #243 Add option to enter/skip onLoop() for Sensor #398 )
• The new Timer's options can be accessed by the user with the new Sensor's methods setReportTimerMode(), setReportTimerValue(), setMeasureTimerMode() and setMeasureTimerValue(). Other functions like setReportInterval*() have been kept for compatibility and simply calls setReportTimerMode(TIME_INTERVAL) and setReportTimerValue(seconds)
• Added Sensor::onOTAConfiguration(ConfigurationRequest* request) which can be implemented by each sensor to support OTA configuration requests
• Implemented a default Sensor::onReceive(MyMessage* message) since the same code was used for most of the sensors which can be still overridden by each sensor. When a child receive a REQ message and the type matches the type of the request, executes its onLoop function. Removed onReceive() from all the sensors using the default logic
• Added setInterruptStrict() to Sensor. When true (the default), the value of the pin is checked and the interrupt ignored if RISING and not HIGH or FALLING and not LOW (fixes For interrupt-based sensor add global option to verify value #352)
• Moved setInitialValue() (renamed into setPinInitialValue()), setInterruptMode(), setWaitAfterInterrupt() from SensorInterrupt and SensorPulseMeter into Sensor and their logic into Sensor::setup() so that present and future interrupt-based sensors can use them
• Deprecated SensorInterrupt::setArmed() since can be emulated by the new Timer class
• Deprecated Sensor::onBefore() since no sensors was using it so to save additional storage. Debug output is no more printing the registration of the sensors since redundant with the presentation in the network
• Added getDisplay() method to Display subclasses so the user can have access to the LCD object for advanced configurations if needed (fixes Make LCD display object accessible in Display class and subclasses #367)

Child API:

• Deprecated Child subclasses ChildInt, ChildFloat, ChildDouble and ChildString. All the required conversion now happens transparently within the Child class which takes as an additional parameter the value format (INT, FLOAT, DOUBLE, STRING). This simplify the code (avoid casting Child to the right subclass for every sensor) and save an additional 500+ bytes of storage.
• Child::setValue() now prints out the value just set, together with the child description and id. In this way there is no more the need to have a manual debug output for each sensor
• Child (apart from STRING format) support keeping track of the last value assigned in EEPROM and restoring it upon a reboot when NODEMANAGER_EEPROM is ON and setPersistValue() is set to true. Particularly useful when e.g. the sensor is counting and a power outage takes place or for restoring the last value set to a relay. Enabled by default on SensorPulseMeter and subclasses (fixes Allow SensorPulseMeter and subclasses to persist counters in EEPROM #304) and in SensorDigitalOutput and subclasses
• Added setValueProcessing() to Child to configure the behavior of the child when setValue() is called multiple times. It can be NONE (ignore the previous values but the last one), AVG (averages the values), SUM (sum up the values) (default: AVG). Adapted SensorPulseMeter and its subclasses to use setValueProcessing(SUM) and output class to use setValueProcessing(NONE)
• Child's last value can be now accessed by other sensors or hook function through getLastValueInt(),
  getLastValueFloat(), getLastValueDouble(), getLastValueString(), provided NODEMANAGER_CONDITIONAL_REPORT is set to ON
• Added setSendWithoutValue() which allow to send the value to the gateway even if no samples have been collected. Useful for sensors like Rain Gauge which has to report anyway at the end of the cycle
• Renamed Child's setValue*() and in just setValue()

Timer class:

• Completely reviewed and optimized the Timer class. A timer can now be configured in different way through setMode() which sets the way the timer operates. It can be either TIME_INTERVAL (e.g. report every X seconds with the amount of time set with setValue()), IMMEDIATELY (e.g. report at every cycle, useful for sensors like actuators which should report as soon as the value has changed), DO_NOT_REPORT (e.g. never report, useful for when there is no need to report, like a Display) and when FEATURE_TIME is ON, EVERY_MINUTE/EVERY_HOUR/EVERY_DAY (e.g. to report the value set in the previous timeframe, useful for sensors reporting an accumulated value linked to a timeframe at regular intervals), AT_MINUTE/AT_HOUR/AT_DAY (e.g. report at a given minute/hour/day, useful if the measure is expected at a specified time, set with setValue()) (fixes Add ability to report at a configured minute of the hour #394 Add ability to unset the reporting timer #357)
• Now Timer stores the provided value as unsigned long and no more as signed int so allowing long timeframe to be honored correctly (fixes Timer fails to honor a value of more than one day #403)
• Timer's update() is no more called by Sensor but when a timer is created it registers itself against the NodeManager class which is responsible to call it on each register timer (fixes Timer elapsed time not accurate #404)

Other changes:

• Renamed Request class into ConfigurationRequest since used only by SensorConfiguration
• ConfigurationRequest now support string input (can be useful for Display class and subclasses)
• Data type refactoring to save additional storage

Including NodeManager's library in your projects

To make use of the library version, replace in the main sketch:

• #include "NodeManagerLibrary.h"
  with:
• #include <MySensors_NodeManager.h>
  This makes (on purpose, to avoid conflicts) NodeManager's v1.7 and v1.8 not compatible

Using both measure and report timer

The following will configure the sensor to take a measure every 10 seconds and report (the calculated average) every 30 seconds

  battery.setReportIntervalSeconds(30);
  battery.setMeasureTimerMode(TIME_INTERVAL);
  battery.setMeasureTimerValue(10);

The following will allow a sensor to take a measure every minute so to be printed on a LCD display but reporting to the network every 5 minutes. Since DisplaySSD1306 calls setReportTimerMode(DO_NOT_REPORT), the screen will be updated at the configured interval:

// temperature will be measured every 60 seconds
  sht21.setMeasureTimerMode(TIME_INTERVAL);
  sht21.setMeasureTimerValue(60);
// temperature will be reported to the network every 5 minutes
  sht21.setReportIntervalMinutes(5);

// the display will "measure" (e.g. run its onLoop) every 60 seconds. Reporting timer is already set to DO_NOT_REPORT
  ssd1306.setMeasureTimerMode(TIME_INTERVAL);
  ssd1306.setMeasureTimerValue(60);

Log Parser Integration

The following regexp can be used to make Log Parser understanding NodeManager's debug output:

{ re: "NM:INIT:VER=(.*)", d: "NodeManager version <b>$1</b>" },
{ re: "NM:INIT:INO=(.*)", d: "Sketch <b>$1</b>" },
{ re: "NM:INIT:LIB VER=(.+) CP=(.+)", d: "MySensors Library version <b>$1</b>, capabilities <b>$2</b>" },
{ re: "NM:INIT:RBT p=(\\d+)", d: "Configured reboot pin <b>$1</b>" },
{ re: "NM:BFR:INIT", d: "Connecting to the gateway..." },
{ re: "NM:BFR:OK", d: "Connection to the gateway successful" },
{ re: "NM:BFR:INT p=(\\d+) m=(\\d+)", d: "Setting up interrupt on pin <b>$1</b> with mode <b>$2</b>" },
{ re: "NM:PRES:(\\w+)\\((\\d+)\\) p=(\\d+) t=(\\d+)", d: "Presented to the gateway <b>child $2</b> for sensor <b>$1</b> as <b>{type:0:$3}</b> with type <b>{type:1:$4}</b>" },
{ re: "NM:STP:ID=(\\d+) M=(\\d+)", d: "This node has id <b>$1</b> and metric is set to <b>$2</b>" },
{ re: "NM:STP:SD T=(\\d+)", d: "Connected SD card reader, type <b>$1</b>" },
{ re: "NM:LOOP:(\\w+)\\((\\d+)\\):SET t=(\\d+) v=(.+)", d: "New value for <b>child $2</b> of sensor <b>$1</b> with <b>{type:1:$3}</b> = <b>$4</b>" },
{ re: "NM:LOOP:INT p=(\\d+) v=(\\d+)", d: "Interrupt received on pin <b>$1</b>, value <b>$2</b>" },
{ re: "NM:LOOP:INPUT\\.\\.\\.", d: "Waiting for input from the serial port" },
{ re: "NM:LOOP:INPUT v=(.*)", d: "Received an input from the serial port: <b>$1</b>" },
{ re: "NM:TIME:REQ", d: "Requesting the time to the controller" },
{ re: "NM:TIME:OK ts=(\\d+)", d: "Received the time from the controller: <b>$1</b>" },
{ re: "NM:SLP:WKP", d: "Wakeup requested" },
{ re: "NM:SLP:SLEEP s=(\\d+)", d: "Going to sleep for <b>$1</b> seconds" },
{ re: "NM:SLP:AWAKE", d: "Waking up from sleep" },
{ re: "NM:SLP:LOAD s=(\\d+)", d: "Loaded configured sleep time: <b>$1</b> seconds" },
{ re: "NM:MSG:SEND\\((\\d+)\\) t=(\\d+) p=(.+)", d: "<b>Child $1</b> sent <b>{type:1:$2}</b> = <b>$3</b>" },
{ re: "NM:MSG:RECV\\((\\d+)\\) c=(\\d+) t=(\\d+) p=(.+)", d: "Received a <b>{command:$2}</b> message for <b>child $1</b> with <b>{type:$2:$3}</b> = <b>$4</b>" },
{ re: "NM:PWR:RBT", d: "Rebooting the node as requested" },
{ re: "NM:PWR:ON p=(\\d+)", d: "Powering <b>on</b> the sensor(s) through pin <b>$1</b>" },
{ re: "NM:PWR:OFF p=(\\d+)", d: "Powering <b>off</b> the sensor(s) through pin <b>$1</b>" },
{ re: "NM:OTA:REQ f=(\\d+) v=(\\d+)", d: "Over-the-air configuration change requested, function <b>$1</b> value <b>$2</b>" },
{ re: "NM:EEPR:CLR", d: "Clearing the EEPROM as requested" },
{ re: "NM:EEPR:LOAD i=(\\d+) v=(\\d+)", d: "Read from EEPROM at position <b>$1</b> the value <b>$2</b>" },
{ re: "NM:EEPR:SAVE i=(\\d+) v=(\\d+)", d: "Wrote to EEPROM at position <b>$1</b> the value <b>$2</b>" },
{ re: "NM:EEPR:(\\w+)\\((\\d+)\\):LOAD", d: "Restoring from EEPROM the value of <b>child $2</b> of sensor <b>$1</b>" },
{ re: "NM:EEPR:(\\w+)\\((\\d+)\\):SAVE", d: "Saving to EEPROM the value of <b>child $2</b> of sensor <b>$1</b" },
{ re: "NM:SENS:([^:]+):(.+)", d: "Sensor <b>$1</b>: $2" },
{ re: "!NM:SENS:([^:]+):(.+)", d: "Error in sensor <b>$1</b>: <b style='color:red'>$2</b>" },

NM_Log Parser Tester.zip

[Akubi]

@user2684,
Hi, I mabye found a way to manage the problems. I need some more days to test and then I will send you a revised version as proposal. Hope you can find some time to review my proposal and make comments.

[user2684]

@Akubi on the EEPROM issue, it should be solved with commit 981308c. The problem was a bit more complex actually and two issues were there. One was as you pointed out with reset() and the other was with _value_processing. _value_processing default is AVG (which fit any sensor taking measures) so DigitalOutput was averaging the values messing up everything. I added setValueProcessing(NONE) to DigitalOutput (and all the other actuators) so only the last value set is retained. As for reset(), resetting the counters is required by any sensor which is taking a measure and start over after reporting so that piace of code has to stay there. BUT when value processing is NONE, there is nothing to do with the counters so that code is not executed. I tested it out and seems to work, let me know if works on your side as well. Looking at the other issues now. Thanks!

[user2684]

@Akubi sorry for spamming first of all. As for the timer issue, I've implemented your recommendation in 7405c13. I'm still not fully convinced it can scale due to the way List is implemented (e.g. with too many push could cause memory issue). But I couldn't find an alternative implementation: the only place where this update can take place is in NodeManager::loop() and there are too many timers around to ask Sensor to update them consistently. We would need to test this a bit to be sure fixing one problem has not introduced another. Btw, this makes me realize v1.7 implementation was buggy as well, since too many timers were not updated consistently (e.g. the safeguard timer of DigitalOutput). Big finding, thanks.

I've also renamed in 52581f5 NodeManager.h and cpp file in Node.h and cpp to avoid the conflict you mentioned. Moving on to the next issue :P

[user2684]

@Akubi getting to your last finding about setForceUpdateTimerValue. Heck you are fully right. I see two alternatives, going to the unsigned long way with an implicit upper bound or adding an additional variable just to keep track of the days. Probably the first is best but I agree with the concerns regarding memory utilization since there are many timers the problem tends to be bigger. Let me know if you found a better approach, otherwise I'll go with this solution. Thanks!

[Akubi]

@user2684, I set up a proposal for solving the timer issue based on unsigned long. Of course memory usage can be an issue. In order to address this I changed memory layout of the NodeManager itself.
So my proposal is based on a copy of August 6. So your last changes are not included.
If you agree with my changes some merge are probably required.
I'm out for the WE, so I send you a copy of my proposal in attach.
Hope that can help and don’t hesitate to ask if any question.

NodeManager-feature-architecture_review v1.8-Dev3.zip

[Akubi]

@user2684, I'm back from my WE. Do you already take a first look to my proposal ?
Just to know if I can continue in that way.
I yes I can merge your last modification and send you a consolidated version.
I know, available time is limited. Don't worry if you don't have time now

[user2684]

@Akubi sorry for the delayed answer (once again) :-/ Yes I reviewed the package, there are a number of interesting things so I took me a while to go through it. First of all the timers, I like your idea to use _target instead of _elapsed. I've implemented it in a similar way with the following little differences:

• removed _last and using _target also for EVERY_HOUR/MINUTE/DAY (so saving an unsigned long)
• in start() I'm not differentiating if already running or not running (if the timer is restarted before being over better to use the current time and not just adding to target)
• update() is called also if NODEMANAGER_TIME is OFF since if on time is reliable and update does nothing

I've also migrated to unsigned long the variables in Timer and NodeManager.
I now want to go through the other changes since I see good things that are worth adding now (e.g. the data type refactoring was in my todo list since the very beginning of this project so I'm super excited to see it there :p)
I know all of this back and forward is slowing down the merge of this PR but there are sono many good things in your work I don't want to miss any if possibile. Thank you again!

[Akubi]

@user2684, your are welcome.
I received some hardware components so this week I will be busy assembling nodes, but please don't hesitate, if I can help in evolving this nice toll that will be a pleasure.
If you have items in the to do list or need help for testing just ask.

[user2684]

@Akubi finally done with the data refactoring, I took a while since has to be done mostly manually. Now this PR and your version should be mostly aligned since included also your changes to sleepBetweenSend, the order in the constants and string support in ConfigurationRequest. Plus this PR already included the most robust logic of the timers implemented a while back. The only things missing are your changes to SensorACS712 and Diplay5110 which would probably better to go through dedicated PRs (once this will be merged). I'll check all of those changes in the next few days so to be sure there is no wrong copy and paste from my side.
One question for your regarding the changes in ConfigurationRequest. Is char _string_data[MAX_PAYLOAD+1]; going to use a huge amount of storage? It doesn't seem so in my test but wonder if I'm just missing something. Thanks

[Akubi]

@user2684, Thank for feedback and for taking in account my proposals.
Concerning SensorACS712 and Diplay5110, yes I agree. Separate PRs.
Concerning ConfigurationRequest, sorry for a mistake, correct declaration for getFunction is :
// return the parsed function
uint8_t getFunction();
Now, the char _string_data[MAX_PAYLOAD+1]; storage usage is 26 bytes. I set this buffer to MAX_PAYLOAD+1 in order to avoid buffer overrun problems.
Of course storage usage must be checked at runtime. This is a member of ConfigurationRequest class and ConfigurationRequest is localy declared in onReceive method of SensorConfiguration class. So the memory is allocated on stack. Not easy to check. I run my memory analysis tool and yes memory is allocated.
I will study if I can integrate in the future the memory analysis system in the debug function of NodeManaged, but this is another story.

[user2684]

After doing some tests I've noticed a few issues especially with Child and sensors not supposed to report periodically (e.g. relays): since _samples was not updated, value was never reported. Fixed it together with a few other minor things. I'm going now to merge this PR since seems kind of working and don't want to stop other PRs to get in. Maybe we can plan a sort of review of this later on, for sure before releasing v1.8.
I've also updated the description with the new things and referenced specific issues for the most relevant changes. Thanks again @Akubi for such valuable contributions!

[user2684]

And regarding memory utilization monitoring, would be great if we can take this somehow into consideration, maybe in a different PR/release. There is too much floating on the stack, the risk something goes wrong with the memory continuously increases :-) (Added #406)

[Akubi]

@user2684, perfect thank. I'm also doing some test and found troubles with CONDITIONAL_REPORT.
But no problems we can now go with specifics PRs

[user2684]

@Akubi FYI, regarding memory utilization, I've just added something very basic to start with in #408 using the built-in function already provided by the MySensors library, just to have a starting point.

[Akubi]

@user2684 OK perfect thank.
I will take a look as soon as possible.
At the moment I'm facing some problems with MySensors 2.3.0
I'm not able to start on a 1MHk node. No parent identification.