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

Modbus QWORD fix #3856

Merged
merged 3 commits into from Oct 12, 2022
Merged

Modbus QWORD fix #3856

merged 3 commits into from Oct 12, 2022

Conversation

dudanov
Copy link
Contributor

@dudanov dudanov commented Sep 29, 2022

What does this implement/fix?

Quick description and explanation of changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes

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

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 @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)

@jpeletier
Copy link
Contributor

Hello @dudanov, is this just code refactoring or what are you actually fixing?

@dudanov
Copy link
Contributor Author

dudanov commented Oct 5, 2022

Hello @dudanov, is this just code refactoring or what are you actually fixing?

Hello. I wrote that this is a fix, not a refactoring. You can see the changes in the code. In short, incorrect register conversion has been fixed.

@jpeletier
Copy link
Contributor

Thank you for the fix. I don't like bureaucracy either, but it helps describing what failed and when, what you are fixing and how in the PR OP, like all other PRs, so it is easier to prioritize without having to look at the code in detail.

btw, clever and succint conversion there, code is much cleaner now.

ESPHome Dev automation moved this from Needs Review to Reviewer Approved Oct 12, 2022
@jesserockz jesserockz added this to the 2022.10.0b2 milestone Oct 12, 2022
@jesserockz jesserockz merged commit 71387be into esphome:dev Oct 12, 2022
ESPHome Dev automation moved this from Reviewer Approved to Done Oct 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2022
ESPHome Dev automation moved this from Done to Reviewer Approved Sep 23, 2023
ESPHome Dev automation moved this from Reviewer Approved to Done Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants