add fallback mode to button actions #355
Conversation
…atch expected backend name
tpanajott
left a comment
tpanajott
left a comment
There was a problem hiding this comment.
Thank you for your PR. This really is a lot of work and a lot of it is good. Please see my review comments for specifics.
| } else if (b1_long_mode == ButtonMode::THERMOSTAT_HEATING) { | ||
| config.set_button1_long_mode(NSPanelConfig_NSPanelButtonMode_THERMOSTAT_HEAT); | ||
| } else if (b1_long_mode == ButtonMode::THERMOSTAT_COOLING) { | ||
| config.set_button1_long_mode(NSPanelConfig_NSPanelButtonMode_THERMOSTAT_COOL); |
There was a problem hiding this comment.
This isn't really an option. It's not the button that is set in thermostat mode but the relay. Ie. you could still have a long press and (we should probably implemented) use the button in short press but only in detached mode. I'm not quite sure as to how to implement that the best way.
There was a problem hiding this comment.
I don't get it, can you explain more, please? This block is a copy of the button1 short press. A long press should have all the same actions the short press can have, shouldn't it?
| } else if (b2_long_mode == ButtonMode::THERMOSTAT_HEATING) { | ||
| config.set_button2_long_mode(NSPanelConfig_NSPanelButtonMode_THERMOSTAT_HEAT); | ||
| } else if (b2_long_mode == ButtonMode::THERMOSTAT_COOLING) { | ||
| config.set_button2_long_mode(NSPanelConfig_NSPanelButtonMode_THERMOSTAT_COOL); |
| this->_rssi = report.rssi(); | ||
| this->_heap_used_pct = report.heap_used_pct(); | ||
| this->_temperature = report.temperature(); | ||
| this->_version = report.version(); |
There was a problem hiding this comment.
Is there a reason you have added the version to the status report? The version of the panel is already sent when the panel registers to the manager.
There was a problem hiding this comment.
I was unsure if and when the version would be updated in the manager. Something is not right in the firmware update parts, see the other comment. With this at least the running version is sent regularly.
Assume this: the user downloads a firmware blob from the website, uses the upload firmware button in the manager, then uses the update firmware button on her panel. Will the panel register again and thus update the version, I'm not sure. Actually I tried to retrace this procedure, I couldn't find any place the version is assigned outside if this change 😳 did the version work before this change?
| {"id", this->_id}, | ||
| {"name", this->_name}, | ||
| {"version", this->_version}, | ||
| {"firmware_md5", this->_current_firmware_md5_checksum}, |
There was a problem hiding this comment.
Why is the firmware md5 checksum being sent to the web UI?
| DETACHED, | ||
| MQTT_PAYLOAD, | ||
| FOLLOW, | ||
| THERMOSTAT_HEATING, |
There was a problem hiding this comment.
Again, these aren't really button modes but relay modes. I can see why you've added them here as to be able to cast the int from the DB into a mode but in that case, why not already use the enum provided by protobuf as to have one less place to update?
There was a problem hiding this comment.
that was an oversight, I try to push an update tonight
| /* | ||
| * handle button presses | ||
| */ | ||
| void handle_button_pressed_command_callback(ButtonMode button_mode, std::optional<uint64_t> entity_id, std::string topic,std::string payload ); |
There was a problem hiding this comment.
Perhaps make topic and payload optional as well as we don't really have any values most of the time.
| enum NSPanelButtonFallbackMode { | ||
| DISABLED=0; | ||
| TOGGLE_RELAY1 = 1; | ||
| TOGLLE_RELAY2 = 2; |
There was a problem hiding this comment.
Is there a reason for why this is not a simple boolean? We really don't need toggle_relay1 and 2 as this logic is all handled within the panel and the panel know what button was pressed. toggle relay 1 if button 1 was pressed, toggle relay 2 in case reverse_relays is enabled.
There was a problem hiding this comment.
From my point of view the flexibility made sense, here all lights are attached to relay1. Buton1 and Button2 run different scenes on the lights in regular situations. If MQTT becomes unavailable the fallback for both buttons is to toggle relay1 to power off/on all lights (which turn on after power loss). Probably also personal preference: 'reversing relay' is less explicit than the choice to toggle either relay on any action (or even both 🤐).
| THERMOSTAT_COOL = 4; | ||
| }; | ||
|
|
||
| enum NSPanelButtonFallbackMode { |
There was a problem hiding this comment.
See comment above about using a boolean instead.
| <span class="icon me-1"> | ||
| <i class="mdi mdi-source-branch"></i> | ||
| </span> | ||
| Fw md5: <span id="firmware-md5-${nspanel_id}">?</span> |
There was a problem hiding this comment.
Is there a reason you want the firmware md5 checksum visible in the web interface? Without testing it really feels like it will clutter the web interface quite substantially.
There was a problem hiding this comment.
Well, it's complicated: Something is not working completely/correctly with the update mechanism. On my setup the panelmanager complained a lot about md5sum being calculated, not being calculated, not matching and thus recommending firmware update. As a power user I wanted to retrace the software's steps and that meant a way to see the md5sum. At that time there was no version to be shown either. And then it kind of stayed.
2 participants
this PR has a companion PR in NSPanelManager: NSPManager/NSPanelManager-firmware#5
This adds a fallback mode to the each button action. In case MQTT is not available fallback can be configured to toggle a relays. This basically enables emergency operation if the home automation is disrupted