thingsboard-gateway icon indicating copy to clipboard operation
thingsboard-gateway copied to clipboard

Fix modbus serial

Open palandri opened this issue 10 months ago • 7 comments

This PR reflects my changes to the modbus connector to work in a serial environment. I tried to maintain the overall architecture of the current code, but changed the logic to work with ModbusSparseDataBlocks.

Please, if possible, give some feedback. I'm running this implementantion atm in my local test setup.

Should close #1672.

palandri avatar Feb 11 '25 21:02 palandri

Marked as a draft, waiting for feedback.

palandri avatar Feb 11 '25 21:02 palandri

Hi @palandri,

Please check my comment.

Also, the following questions appeared:

  1. If data on the server was modified by external master, will it be updated in data block?
  2. If we modify the data after server starting - will external master receive new data or still retrieve the old?

imbeacon avatar Feb 12 '25 06:02 imbeacon

Hello, @imbeacon, thanks for your feedback.

If data on the server was modified by external master, will it be updated in data block?

Yes, it will. When a master writes to the server, it calls ModbusSparseDataBlock.setValues internally, overridden here, triggering AsyncModbusConnector callback, which finally enter the older dataflow (tested and proven) and send data upstream.

If we modify the data after server starting - will external master receive new data or still retrieve the old?

Data is correctly written/retrieved in gateway scope, I did not test in Thingsboard scope. For example, if another entity (on Thingsboard context) change an attribute of the corresponding device connected over the gateway, will a Client reading the Server fetch this new data?

I'll test and report it.

palandri avatar Feb 12 '25 14:02 palandri

Reporting my testings.

Gateway freshly started, datablocks is initialized with zero, which master on the bus reads. This data does not propagate upstream, in Thingsboard device shows the last attribute/telemetry (sm is mapped to register 1 of the datablock).

  • Data read from master image

  • Data shown on Thingsboard (3 is an older data) image

Master writes to Server, updating datablock (which can be read by master again) which is propagated upstream.

  • Master written (and then reading) from Server image
  • Thingsboard showing data updated image

For example, if another entity (on Thingsboard context) change an attribute of the corresponding device connected over the gateway, will a Client reading the Server fetch this new data?

Turns out gateway maps attributes as Client-side (which is correct), so you can't modify attributes in Thingsboard (which makes sense). So, attributes/telemetry do not propagate downstream, only upstream.

palandri avatar Feb 12 '25 14:02 palandri

Looks like it needs to send uplink right after server started to keep value on TB updated.

According to shared attributes updates and RPC - I think we need to adjust shared attributes updates/RPC logic for server in Modbus Connector. You can do this or we will do this after merge, it is not a problem.

Thank you, we appreciate your contribution.

imbeacon avatar Feb 13 '25 05:02 imbeacon

Looks like it needs to send uplink right after server started to keep value on TB updated. According to shared attributes updates and RPC - I think we need to adjust shared attributes updates/RPC logic for server in Modbus Connector. You can do this or we will do this after merge, it is not a problem.

Oh, I see. To be honest, I did not test shared attributes updates/RPC, only attributes and timeseries. I'll test it and propose changes, no problem.

Turns out gateway maps attributes as Client-side (which is correct), so you can't modify attributes in Thingsboard (which makes sense). So, attributes/telemetry do not propagate downstream, only upstream.

The reported behaviour is specific to attributes/telemetry, not shared attributes. So, as I'm understanding, this behaviour is correct. Could you please confirm, @imbeacon?

Also, about the overall implementation and/or style, could you share some constructive feedback? I'm always looking for opportunities to grow as a developer.

palandri avatar Feb 13 '25 16:02 palandri

Hi @palandri,

First of all, I appreciate your contribution and the effort you’ve put into improving the ModbusConnector!

The reported behavior is specific to attributes/telemetry, not shared attributes. So, as I’m understanding, this behavior is correct. Could you please confirm, @imbeacon?

If you’re referring to updating values right after the server starts, then yes, that behavior is correct.

Regarding the overall implementation, I agree with your approach, but I believe we can simplify things further. Specifically, we can eliminate the need for the no_master parameter when the gateway operates as a server. Since we can now read the actual state of sparse blocks and update them without requiring a client, keeping the client as-is may lead to issues—especially with shared attribute updates or incoming RPCs, where an inability to connect to the serial port could arise.

A more effective approach would be to remove the client when the gateway operates as a server across all communication types (Serial/TCP/UDP). This way, the Slave class wouldn’t need any modifications, as it wouldn’t be used in this scenario. Instead, all necessary adjustments would be contained within the Server class and ModbusConnector.

Does this approach make sense to you? While it’s slightly more complex than the initial idea, it provides a cleaner abstraction and avoids unnecessary client creation, making the implementation more efficient in the long run.

Looking forward to your thoughts!

imbeacon avatar Feb 25 '25 05:02 imbeacon