mysensors / NodeManager

Plugin for a rapid development of battery-powered sensors
130 stars 82 forks source link

Changes for PowerManager #540

Closed j54n1n closed 2 years ago

j54n1n commented 2 years ago

Hello,

I would like to propose two changes for the PowerManager.

First I found that when NodeManager logging is enabled and someone is using the PowerManager to switch only the VCC pin for example with:

#define PIN_GND -1
#define PIN_VCC 12
PowerManager pm{PIN_GND, PIN_VCC};
...
nodeManager.setPowerManager(pm);

the log would show up like this:

44816 NM:PWR:ON p=12
...
50981 NM:PWR:OFF p=-1

The log is basically showing that the invalid GND pin gets switched off. This is actually only a cosmetic error, because if I understood the code correctly this is working as intended, since it is switching only the VCC pin off regardless if the GND pin gets used or not.

So my first proposal would be to change debug log in line 61 such that the _vcc_pin is logged instead:

debug(PSTR(LOG_POWER "OFF p=%d\n"),_vcc_pin);

The second case involves subclassing of the PowerManager class. Currently any subclass of the PowerManager does not allow the overriding of the methods setPowerPins, powerOn, and powerOff since the are not virtual. Same applies also for the access modifier of the member variables that are for now private to the class.

One of my use cases would include the switching of multiple sensors VCC pins at the same time. I am aware that the PowerManager can be applied to a sensor individually but by doing so it happens in a serial fashion one at a time like so:

36098 NM:PWR:ON p=20
37169 NM:LOOP:DW-6490(1):SET t=37 v=7.32
37173 NM:PWR:OFF p=-1
37184 NM:PWR:ON p=21
38256 NM:LOOP:DW-6450(2):SET t=37 v=686.62
38260 NM:PWR:OFF p=-1
38296 NM:PWR:ON p=24
39510 NM:LOOP:DW-7346(6):SET t=0 v=-40.00
41837 NM:LOOP:DW-7346(7):SET t=1 v=-4.-68
41841 NM:PWR:OFF p=-1

So therefore using a subclassed PowerManager applied on the nodeManager object would me allow to do this where I could override the powerOn and powerOff methods to switch all the pins at the same time during one loop cycle. To be more general this would need the virtual and protected specifiers on the NodeManager class.

Since my sensors need a long time to startup I would like to unify the startup time with a single PowerManager, something like this:

36098 NM:PWR:ON ...
37169 NM:LOOP:DW-6490(1):SET t=37 v=7.32
37256 NM:LOOP:DW-6450(2):SET t=37 v=686.62
37510 NM:LOOP:DW-7346(6):SET t=0 v=-40.00
37837 NM:LOOP:DW-7346(7):SET t=1 v=-4.-68
37841 NM:PWR:OFF ...

Let me know if such a change is OK.

user2684 commented 2 years ago

Hi! Thanks for the contribution first of all! The first item you mention is actually a mistake, the vcc_pin has to be printed out not the ground pin, good catch! For the second item, I agree on the approach, if there are use cases for subclassing the PowerManager class, this should be made possible. I'm going to merge your PR into the development branch in a sec. Thanks again!