Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow reading/writing address 0 in the simulator #1552

Merged
merged 3 commits into from
May 28, 2023
Merged

Allow reading/writing address 0 in the simulator #1552

merged 3 commits into from
May 28, 2023

Conversation

corollaries
Copy link
Contributor

@corollaries corollaries commented May 27, 2023

This PR is related to #1551 and #1472. Currently trying to attempt to read coils from bit 0 to 15 is not possible in the simulator and will result in an invalid address error. These bits get converted to the real address, which for bit 0 to 15 would be zero, and thus results in the invalid address error because of validate() in simulator.py. This is not only related to read_coil, but also other functions fail on address 0 while technically it should be possible to execute.

From reading the documentation on all functions it seems that the API expects addresses and not registers, which leads to the expectation that address 0 (or bit 0-15) should be readable. The simulator also accepts setting address 0 in the configuration, but in it's current state it's not possible to read or write to this address.

@janiversen
Copy link
Collaborator

Technically register 0 do not exist, some very old devices use it, but the simulator do not support it.

If you define shared memory first possible coil is bit 16 (register 1). If you have separate blocks you should be able to do a read_coil of bit 0.

Which device are you trying to simulate that uses register 0 ?

@janiversen
Copy link
Collaborator

btw the API clearly talks about registers or register address not addresses, because the modbus protocol only talks about registers, please see the simulator documentation where there are a drawing explaining the difference.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need some very convincing arguments why this should be allowed.

real_address = self.fc_offset[func_code] + address
if real_address <= 0 or real_address > self.register_count:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct we do not use address 0, nor do we use register 0.

@corollaries
Copy link
Contributor Author

@janiversen

Thanks for the review.

Which device are you trying to simulate that uses register 0 ?

In our case we are using the simulator to test a client. The simulator isn't necessarily used to "reflect" a real world device, just to verify whether a client works correctly or not. Based on many examples / documentation we expected to be able to read/write on address 0.

btw the API clearly talks about registers or register address not addresses

If I look at the documentation for read_coils, read_holding_registers, etc the parameters are always address, hence the assumption that we're talking about addresses and not registers . As far as I can tell from the code these can be 0-indexed. Just not in combination with the simulator.
Example for read_coil docs:

In the PDU Coils are addressed starting at zero. Therefore coils
numbered 1-16 are addressed as 0-15.

The simulator in turn uses addr which are actually registers. It just makes using the simulator and the client IMO very confusing. Also given that we can actually set "addr": 0 in the simulator configuration but somehow cannot use it. Even the example on the README of this repository would fail to run with the simulator as it's attempting to read/write coil 1.

I'm fine with dropping this PR but maybe the following might then be a good course of action:

  • Specify it very clearly in the docs that bit 0-15 and address 0 using the client cannot be used in combination with the simulator.
  • Maybe rewrite the simulator configuration to:
"uint16": [
    { "register": 0, "value": 1 } /// making it clear that we're talking about registers here.
]
  • Rewrite examples to not use address bit 0-15. People new to this library and/or modbus might copy examples to test something out real quick, to then run into trouble is confusing.

Hope this explains the rationale a bit.

@janiversen
Copy link
Collaborator

It is true that both the modbus documentation and our API uses "register addresses" which are not the same as memory addresses (hence the reference to the official drawing). The difference between memory addresses and modbus protocol register addresses are indeed confusing, but see it as 2 different worlds that have 0 in common a server can map the register address to whatever memory it wants.

There are a lot of examples also in the real world that allows you to read coil 0, but in all these examples you do not have shared blocks, did you try to configure without shared blocks.

We have tried to be very clear in the documentation, especially regarding the difference between shared and non-shared blocks, which HIGHLY affects how to address registers and coils, but corrections are always welcome.

I have updated your branch to match the current dev, because I think you miss a couple of changes we made to the simulator (and other parts).

@corollaries
Copy link
Contributor Author

did you try to configure without shared blocks.

Just tested it and it fails there as well. In the end the validate() method is always called, for bits 0-15 the real_address will be always be 0 and thus an InvalidAddress will be given. In whatever configuration we currently set the simulator, coils 0-15 will not work.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I highly disagree, but admitted we have no tests to prove it is wrong.

I cannot guarantee this functionality will be kept when we do an overhaul of the simulator configuration (currently only ideas on a drawing board).

@janiversen janiversen merged commit ecbe96b into pymodbus-dev:dev May 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants