Fix modbus serial

[palandri]

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]

Marked as a draft, waiting for feedback.

[imbeacon]

It looks like it always be 'no_master': True. Because you at first unpack config and then overwrite no_master it with True.

Also are you sure that we actually need no_master parameter in general? Because if connector can reads data from data blocks - master will be no longer required

[palandri]

Yes, I think you are right in this regard. I tried to modify as little as possible of the Slave class, to not break stuff, so I created the 'no_master' parameter to keep the current implementantion of the Slave class working in polling mode. But it isnt a parameter of the Server class, but Slave class, initialized here. When get_slave_config_format is called, it creates a Slave object (inside Server context), with no_master = True. If the Slave object is called outside of Server context, no_master do not exists, so it defaults to False.

With the gateway working as a Client, it makes sense to poll Servers (which is triggered by the Slave-class object).
On the other hand, with the gateway working as a Server, polling doesn't make sense anymore (as the Server need to react to when data is sent to it).

So, 'no_master' simply control if Slave' internal timer is to be activated or not.

I think a better implementation of it would be to change Slave class to initialize the timer based on poll_period parameter (for example, if poll_period = 0, do not poll).

[imbeacon]

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?

[palandri]

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]

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
  

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

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

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

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.

[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.

Thank you, we appreciate your contribution.

[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.

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.

[imbeacon]

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!