Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Unnecessary flash writes by ModbusSwitch component #3648

Merged
merged 13 commits into from
Nov 29, 2022

Conversation

jpeletier
Copy link
Contributor

@jpeletier jpeletier commented Jul 22, 2022

What does this implement/fix?

Modbus switches are unnecessarily updating their status to flash, since the actual state resides in the modbus slave device.

This save state behavior resides in the base Switch class in publish_state(), therefore ModbusSwitch could not disable saving to flash. In fact, ModbusSwitch had to hack its way out of this, being forced to do a dummy read of initial_state even though it is not necessary for ModbusSwitch.

When using a large number of Modbus switches, these end up unnecessarily updating their status to flash, causing flash wear, or even a boot loop when saving preferences.

This PR addresses the problem the following way:

  • Extracts the restore_mode functionality out of GpioSwitch and OutputSwitch, moving it to Switch
  • Has ModbusSwitch restore mode default to ALWAYS_OFF: starts as OFF until the first modbus refresh, which is the way it is working now.
  • Refactors GpioSwitch and OutputSwitch to rely on Switch for restore mode configuration.

The change is backward compatible. current Switch-based components are not affected.

However, any Switch-based component can optionally leverage restore mode. For example, ModbusSwitch could be easily rewritten to actually persist changes to flash, so that the state is restored and held until the first modbus refresh takes place. Other components, such as TemplateSwitch could deprecate restore_state and such, in favor of the built-in Switch restore functionality.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#2381

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (gpio) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (output) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-esphome
Copy link

Hey there @martgras, mind taking a look at this pull request as it has been labeled with an integration (modbus_controller) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (switch) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@jpeletier
Copy link
Contributor Author

@jesserockz Can we finish this review? I had recently to rebase due to a number of conflicts, I would like to avoid too much divergence, this fix is important to avoid flash wear. Thanks!

@jpeletier
Copy link
Contributor Author

Here is the PR for the related documentation: esphome/esphome-docs#2381

@nagyrobi
Copy link
Member

My relay card operation is not very consistent so I'd rather rely on ESPHome restore functionality to always forcefully restore the modbus switches to what they were previously. For example when there's a power cycle, the Modbus device doesn't restore the relays states by itself, it would be very handy to restore them from ESPHome.

Do I assume correctly that with RESTORE_DEFAULT_OFF I could achieve what I want?

@nagyrobi
Copy link
Member

nagyrobi commented Nov 28, 2022

Do I assume correctly that with RESTORE_DEFAULT_OFF I could achieve what I want?

Answering myself, no. Tested this PR with:

external_components:
  - source: github://pr#3648
    components: [ switch, output, modbus_controller, gpio ]
    refresh: 1min

And config:

switch:
- platform: modbus_controller
  modbus_controller_id: relay24
  id: heat_00_sw
  name: 00 switch
  register_type: coil
  address: 0
  bitmask: 1
  restore_mode: RESTORE_DEFAULT_OFF
  1. Turned the the switch on
  2. Waited 2 minutes (to ensure saving to flash cycle is reached at least once)
  3. Power-cycled the entire circuit (modbus relay card and ESPHome node)
  4. Result: switch didn't turn back on as exepcted with RESTORE_DEFAULT_OFF

The expectation was that the board should have saved the switch state to the flash (didn't see an entry in the log about that, either) and restore it after reboot.

@jpeletier jpeletier force-pushed the switch-restore-mode branch 2 times, most recently from 4cefdbb to 603bc53 Compare November 29, 2022 12:11
@jpeletier
Copy link
Contributor Author

Hello @nagyrobi,

Indeed this PR does not do what you expect. Note that it was not the original purpose of this PR, which was to fix some flash writes that were not applicable to the modbus component and that were causing unnecessary flash wear.

However, this PR does lay the groundwork to quickly build the feature you need. Hopefully I can do that once this is merged.

In the meantime, I have updated the documentation: https://deploy-preview-2381--esphome.netlify.app/components/switch/modbus_controller.html based on your feedback, to avoid confusion.

Regards

@nagyrobi
Copy link
Member

this PR does lay the groundwork to quickly build the feature you need. Hopefully I can do that once this is merged

I would really appreciate if it would be possible to merge all in one, in case it's not too complicated. Since I have a real use case, I can contribute with real world tests in exchange for your work.

@jpeletier
Copy link
Contributor Author

This PR is already too big, and those requirements out of its scope. Let's push it to be merged as soon as possible, so we can add that functionality.

In the meantime, a boot script should fix your problem: https://esphome.io/components/esphome.html#on-boot

@jesserockz jesserockz merged commit c55e01f into esphome:dev Nov 29, 2022
@jpeletier jpeletier deleted the switch-restore-mode branch November 29, 2022 21:07
@jpeletier
Copy link
Contributor Author

Thanks @jesserockz

@nagyrobi can you test #4122 and see if it solves your issue?

jesserockz pushed a commit that referenced this pull request Nov 30, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2022
@kbx81
Copy link
Member

kbx81 commented Dec 6, 2022

@jpeletier could you have a look at this? esphome/issues#3878 😇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants