-
-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
Ignore size check for DataType.CUSTOM in modbus #101523
Conversation
Hey there @janiversen, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
byte_count is calculated by the pack functions, also in case of datatype custom, so the check is needed to avoid a runtime exception, when struct is defined wrongly.
Please add tests that show a custom struct configured wrongly do not cause a runtime exception.
Just for information, the documentation states: quote "The integration adheres strictly to the protocol specification for the actual protocol implementation." so non-standard is not supported, but if it works it's fine. Any PR that implement a non-standard functionality and do not put the standard at risk (by e.g. provoking a runtime error in case of a wrong configuration) will be accepted. |
I know. I only try apply for custom and maybe string. Current test fail. After you change count this is needed. Current modbus component if have issues creates a delay in boot/restart from 1min to 5min. Personally I avoid to use modbus component, but some users still need it Any plans in modbus standards for an update? The funny thing, the issue is only in request for almost implementations. I tried fix this "32bits count 1" everywhere. Regards |
Yes current test fails, please implement changes according to the review comments. The tests are there for a reason, so if a test fails it because something is not working as it should. |
Anything happening on this PR, if no response it will be closed in 72 hours. |
and that means ? |
I currently don't have time |
as we have seen before....but thanks for trying. |
Breaking change
Proposed change
Last count changes breaked non standard 32 bits modbus
PR result:
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: