tjhowse / modbus4mqtt

Modbus TCP <-> MQTT glue. YAML configuration. Robust.
Other
75 stars 33 forks source link

Multi-byte read #29

Closed icypete closed 1 year ago

icypete commented 3 years ago

Added support for reading multi-byte data.

Essentially register for enough sequential address to fill the register type.

Then read enough registers to build data.

Adjusted the masking to only be applied to uint types (I don't think masking any other types makes sense).

codecov[bot] commented 3 years ago

Codecov Report

Merging #29 (b24cf4e) into master (81102a6) will increase coverage by 0.59%. The diff coverage is 98.67%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   95.83%   96.43%   +0.59%     
==========================================
  Files           6        6              
  Lines         841     1010     +169     
==========================================
+ Hits          806      974     +168     
- Misses         35       36       +1     
Impacted Files Coverage Δ
tests/test_modbus.py 96.52% <97.70%> (-0.62%) :arrow_down:
modbus4mqtt/modbus4mqtt.py 94.55% <100.00%> (+0.05%) :arrow_up:
modbus4mqtt/modbus_interface.py 96.35% <100.00%> (+0.77%) :arrow_up:
modbus4mqtt/version.py 100.00% <100.00%> (ø)
tests/test_mqtt.py 98.67% <100.00%> (+1.26%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tjhowse commented 3 years ago

Thanks very much for doing this work! It makes me feel warm and fuzzy that A) you found my project useful, and B) you are actively helping to make it better. There are a few changes I'd like you to make to this PR to make it perfect. If you haven't been through a code review process before (or even if you have!) this can be a bit dispiriting, but I think the end result will be worth the effort. I found this site to be a good start as a reference guide for raising high-quality pull requests: https://mtlynch.io/code-review-love/ Thanks again!

tjhowse commented 3 years ago

The last big thing this PR needs is some more testing to cover the new code. The most straightforward way of doing this would be adding the new types to test_mqtt.test_type and test_type.yaml. In test_mqtt we define a list of dummy modbus register contents, and then we query those with a poll() and check what values were "published" to our mock MQTT interface based on the registers configured in test_type.yaml. Testing these new types would be a matter of expanding the list of dummy modbus table data, and the registers in the test YAML, to demonstrate the conversions from uint16 to each of the new types work as expected.

Writing these tests strings up a safety net to catch future changes that might break the functionality this PR adds.

tjhowse commented 3 years ago

Ah! We'll also need to update the readme with the new supported register types, and note in the mask row of the table that it is only supported for use with uint16 register types.

icypete commented 3 years ago

Thanks for the comments! I shall get on to them later this week. (I like feedback in all forms, it is how we learn :) )

icypete commented 3 years ago

Got a bit sidetracked on another project. Hopefully will get to this by the end of the week

tjhowse commented 3 years ago

No worries at all, I know how it is. :)

iconnor commented 3 years ago

With the pep8 changes I just made in #32, I think this would be good to merge first. If not, I am happy to make these changes after #32 if it helps.

tjhowse commented 3 years ago

No worries, I'll fix up this PR after #32 is merged.

pep8speaks commented 1 year ago

Hello @icypete! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 68:121: E501 line too long (128 > 120 characters) Line 105:44: E201 whitespace after '('

Line 17:1: E302 expected 2 blank lines, found 1 Line 23:121: E501 line too long (136 > 120 characters) Line 106:37: E231 missing whitespace after ',' Line 179:1: E302 expected 2 blank lines, found 1 Line 188:21: E211 whitespace before '(' Line 190:1: E302 expected 2 blank lines, found 1 Line 196:21: E211 whitespace before '(' Line 198:1: E302 expected 2 blank lines, found 1 Line 201:32: E231 missing whitespace after ',' Line 201:48: E231 missing whitespace after ',' Line 203:1: E302 expected 2 blank lines, found 1 Line 208:51: E231 missing whitespace after ',' Line 208:67: E231 missing whitespace after ','

Line 245:9: E722 do not use bare 'except' Line 250:9: E722 do not use bare 'except' Line 255:9: E722 do not use bare 'except' Line 265:121: E501 line too long (143 > 120 characters) Line 267:29: E231 missing whitespace after ',' Line 281:87: E231 missing whitespace after ',' Line 282:87: E231 missing whitespace after ',' Line 288:87: E231 missing whitespace after ',' Line 289:87: E231 missing whitespace after ',' Line 290:87: E231 missing whitespace after ',' Line 291:87: E231 missing whitespace after ',' Line 300:121: E501 line too long (143 > 120 characters) Line 302:29: E231 missing whitespace after ',' Line 307:87: E231 missing whitespace after ',' Line 308:87: E231 missing whitespace after ',' Line 314:87: E231 missing whitespace after ',' Line 315:87: E231 missing whitespace after ',' Line 316:87: E231 missing whitespace after ',' Line 317:87: E231 missing whitespace after ',' Line 328:121: E501 line too long (143 > 120 characters) Line 330:29: E231 missing whitespace after ',' Line 363:121: E501 line too long (132 > 120 characters) Line 365:121: E501 line too long (143 > 120 characters) Line 367:29: E231 missing whitespace after ','

Line 36:36: E231 missing whitespace after ',' Line 502:121: E501 line too long (129 > 120 characters) Line 504:121: E501 line too long (160 > 120 characters) Line 513:121: E501 line too long (135 > 120 characters) Line 515:121: E501 line too long (160 > 120 characters) Line 524:121: E501 line too long (144 > 120 characters) Line 526:121: E501 line too long (160 > 120 characters)

Comment last updated at 2022-08-24 06:26:51 UTC