search

espressif / esp-modbus

ESP-Modbus - the officially suppported library for Modbus protocol (serial RS485 + TCP over WiFi or Ethernet).
Apache License 2.0
107 stars 49 forks source link

Modbus coils start at 0th bit tcp (IDFGH-10998) #35

Closed amateusz closed 1 year ago

amateusz commented 1 year ago

Coils/discrete transactions (read/write) seem to align everything to %8, so they work correctly only for start address being n*8. This is not specified in the standard. Affects both RTU and TCP.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

alisitsyn commented 1 year ago

Hi @amateusz,

Thank you for your PR. The commit with the fix to shift the coils readout by n%8 is confirmed. The additional commit to fix it in the tcp master will be added. The PR starts following the formal process.

amateusz commented 1 year ago

Oh! I've created a PR by mistake, but I guess I'll just stick to it. I indeed think there is a mistake in the esp-modbus reporting of coils/discrete params, that is is shifted by n%8. Please excuse the messy commits and let me clean it up (remove unnecessary commits). btw. I have TCP + RTU "patched" the same way.

alisitsyn commented 1 year ago

I indeed think there is a mistake in the esp-modbus reporting of coils/discrete params, that is is shifted by n%8.

Yes, I agree that the fix is required. This was not visible when the coils are read at an 8 bit boundary.

Please excuse the messy commits and let me clean it up (remove unnecessary commits).

It is ok. I already took your commit and added the required fixes in separate one. You can also clean up and update the commit to use it as is.

Thanks.

amateusz commented 1 year ago

I cleaned it up. Now it's just a the minimum change. It covers just coils+discrete parameter type in both RTU+TCP master mode.

alisitsyn commented 1 year ago

@amateusz,

Thank you for the update. Your changes in the commit will be included to the MR and mentioned here once merged. This will take some time to complete other merge requests to release new version of the component.

amateusz commented 1 year ago

Thank you :) I’m curious though: has this issue (for not aligned coils transactions) not surfaced in real world nor in the assumed internal tests until now..? I triple checked the standard and zephyr implementation when I stumbled upon this..

alisitsyn commented 1 year ago

I’m curious though: has this issue (for not aligned coils transactions) not surfaced in real world nor in the assumed internal tests until now..?

The issue was found a long time ago under esp-idf modbus component. However, the fix was not merged due to some internal aspects. The internal tests cover the coils read/write aligned to 8 bits and issue has not been observed.

alisitsyn commented 1 year ago

The PR is merged in Release v1.0.12 with commit 8dadaac490c492d2f5f66243d00c91db2d30e31b.