Create _P164_MSswitch.ino #130
Conversation
Plugin to control Nexa, Prove, Telldus & Jula Anslutt socket switches
|
|
||
| // Download library from https://github.com/mortensalomon/Socket_Switch | ||
|
|
||
| Socketswitch *Plugin_164_msswitch; |
There was a problem hiding this comment.
This must be initialized to nullptr, or else you will get unexpected behavior
| if (CONFIG_PIN1>-1) { | ||
| addFormSubHeader(F("Try it")); | ||
| String options[17] = { F("CH 1"),F("CH 2"),F("CH 3"),F("CH 4"),F("CH 5"),F("CH 6"),F("CH 7"),F("CH 8"),F("CH 9"),F("CH 10"),F("CH 11"),F("CH 12"),F("CH 13"),F("CH 14"),F("CH 15"),F("CH 16"),F("GROUP") }; | ||
| int optionValues[17] = { 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16 }; |
There was a problem hiding this comment.
Why not use a for loop for this?
These strings also add to the size of the binary.
| #define PLUGIN_VALUENAME1_164 "" | ||
|
|
||
| unsigned long SocketAddress; | ||
| String log164; |
There was a problem hiding this comment.
Do not declare this global. Better to declare it inside the function, then you know for sure the string will be destructed when the function has finished.
Try to keep the scope of variables as small as possible, then it is easier to predict their behavior.
A string that will remain allocated can grow and keep the memory allocated even when it is no longer needed, so it will keep its largest allocated size.
| noInterrupts(); | ||
| if (getFormItemInt(F("state"))) { | ||
| if (getFormItemInt(F("channel"))==16) { | ||
| Plugin_164_msswitch->groupOn(SocketAddress); |
There was a problem hiding this comment.
There is no check to see if Plugin_164_msswitch is a valid pointer.
This means it will lead to a crash when the plugin is saved while not being enabled.
Please check other uses of this pointer.
| { | ||
| if (!Plugin_164_msswitch) | ||
| { | ||
| Plugin_164_msswitch = new Socketswitch(CONFIG_PIN1); |
There was a problem hiding this comment.
You have implemented a PLUGIN_INIT but no PLUGIN_EXIT in which the pointer must be deleted.
This means this allocation may lead to a memory leak when the plugin is removed.
Also a change of pin settings now need a reboot of the ESP to activate.
| if (val_state.equalsIgnoreCase("on")) Plugin_164_msswitch->groupOn(SocketAddress); | ||
| if (val_state.equalsIgnoreCase("off")) Plugin_164_msswitch->groupOff(SocketAddress); | ||
| } else { | ||
| if (val_dev == "1" || val_dev == "2" || val_dev == "3" || val_dev == "4" || val_dev == "5" || val_dev == "6" || val_dev == "7" || val_dev == "8" || val_dev == "9" || val_dev == "10" || val_dev == "11" || val_dev == "12" || val_dev == "13" || val_dev == "14" || val_dev == "15" || val_dev == "16") { |
There was a problem hiding this comment.
We have functions to check if a string is a numerical value. Please use that, then convert to an int and check its range.
These strings all add up to the memory consumption and binary file size.
| #define PLUGIN_NAME_164 "MS auto-learning Switch" | ||
| #define PLUGIN_VALUENAME1_164 "" | ||
|
|
||
| unsigned long SocketAddress; |
There was a problem hiding this comment.
In every location where this value is used, the event variable is present, so you can also directly call PCONFIG_LONG(0) or make a define for it for better readability.
For example something like this:
#define p164_SocketAddress PCONFIG_LONG(0)
Plugin to control Nexa, Prove, Telldus & Jula Anslutt socket switches