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 byte count when writing multiple coils #105

Conversation

acolomb
Copy link
Contributor

@acolomb acolomb commented Aug 26, 2020

Fix off-by-one error in WriteMultipleCoils calculation. For full bytes, the Byte Count field was calculated too high:
8 // 8 + 1 = 2
while only one byte is needed to represent 8 coils.

Offset the values length before the truncating division to correctly
round to whole bytes. This is easier than what the standard
formulates:
N = Quantity of Outputs / 8, if the
remainder is different of 0 ==> N = N+1

Note that the actual data bytes were already handled with the correct
length. So in case of 8 coils, uModbus would send a Byte Count of 2,
but only one data byte before the checksum

For full bytes, the Byte Count field was calculated too high:
    8 // 8 + 1 = 2
while only one byte is needed to represent 8 coils.

Offset the values length before the truncating division to correctly
round to whole bytes.  This is easier than what the standard
formulates:
    N = Quantity of Outputs / 8, if the
    remainder is different of 0 ==> N = N+1

Note that the actual data bytes were already handled with the correct
length.  So in case of 8 coils, uModbus would send a Byte Count of 2,
but only one data byte before the checksum.
Put the Byte Count field after the Quantity, corresponding to the
actual ADU layout.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 95.814% when pulling 4db2279 on acolomb:fix-length-writing-multiple-coils into 63c7679 on AdvancedClimateSystems:master.

Copy link
Collaborator

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@OrangeTux OrangeTux merged commit b928ff4 into AdvancedClimateSystems:master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants