-
Notifications
You must be signed in to change notification settings - Fork 987
Issue 2675 correctly reverse fixed size datatypes #2678
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
Issue 2675 correctly reverse fixed size datatypes #2678
Conversation
The remaining CI fails are related to code prior to my Pull Request so, although the suggested changes sound very reasonable to me, I didn't dare to change it. Let me know if you'd rather have me also change those couple of lines. |
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.
PR looks good apart from one small comment.
But you need to add tests to show it works as expected. We have a demand for 100% test coverage plus functionality test for all rewritten modules.
pymodbus/client/mixin.py
Outdated
@classmethod | ||
def _get_reversed_registers(cls, regs: list[int], data_type: DATATYPE) -> list[int]: | ||
data_type_len = data_type.value[1] | ||
if not data_type_len: |
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.
Please combine the 2 lines using the := operator.
We do not merge anything without a clear CI (and tests depending on the module). |
And just fyi, line 781 in mixin.py is your code ! The other reported error is a side effect that goes away when your code passes. |
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.
LGTM, thanks.
As a response to issue #2675 this PR improves "convert_to_registers" in order to do the reversing of the bytes per word, and not for the complete data array.
String and bit fields might vary in size and way of handling so I didn't dare to change anything about then in this MR, it just works with datatypes that have a fixed known size.
fixes #2675