-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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 sensor refactoring #3297
Modbus sensor refactoring #3297
Conversation
Any chance this can make it into .28? |
add_devices(sensors) | ||
|
||
|
||
class ModbusCoilSensor(Entity): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use BinarySensor instead Entity. See on other binary sensors.
The switch is still to be done. Not ready for merge yet |
The refactoring is done, except for fixes for future review comments. |
vol.Required(CONF_BAUDRATE): cv.positive_int, | ||
vol.Required(CONF_BYTESIZE): vol.Any(5, 6, 7, 8), | ||
vol.Required(CONF_METHOD): vol.Any('rtu', 'ascii'), | ||
vol.Required(CONF_PORT): str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of consistency -> cv.string
Looks good overall, but I do not have any Modbus kit though. Would be nice if someone can help test it, but as long as the upstream library supports |
I found a modbus slave simulation tool, I can do some simple tests with that. |
Managed to find a bug from pre-refactoring with the test tool (http://www.modbusdriver.com/diagslave.html). binary_sensor and switch are tested and works |
Looks good! 🐬 |
Description:
The modbus sensor needed some love.
I have:
I will add voluptuous in the next PR
I have not been able to test the binary_sensor or switch since my unit lacks coils, if anyone has a unit with coils I would appreciate some help!
Related issue (if applicable): fixes #
#3038
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
home-assistant/home-assistant.io#933
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If code communicates with devices, web services, or a:
tox
run successfully. Your PR cannot be merged unless tests pass