Skip to content

Fix byte count when writing multiple coils#105

Merged
OrangeTux merged 2 commits into
AdvancedClimateSystems:masterfrom
acolomb:fix-length-writing-multiple-coils
Aug 27, 2020
Merged

Fix byte count when writing multiple coils#105
OrangeTux merged 2 commits into
AdvancedClimateSystems:masterfrom
acolomb:fix-length-writing-multiple-coils

Conversation

@acolomb

@acolomb acolomb commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown

Coverage Status

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

@OrangeTux OrangeTux left a comment

Copy link
Copy Markdown
Collaborator

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