Fix register type check for size bigger than 3 registers (6 bytes) #1323
Conversation
|
Did you try the test ? please remember what I wrote make a test case thst shows the problem, add your fix and check that the test works. your test does not work with your fix. |
duc996
force-pushed
the
fix_simulator_01
branch
from
f1f978d
to
405c157
Compare
Yes, I run the test, but only the simulator test but other tests are broken. A little side note: who should I start my tests? I used the commend from the CI, but there is also the pylint. Is there any command to start all tests from CI? Also serial-ModbusRtuFramer and serial-ModbusAsciiFramer are failing most of the time on my local PC event without my patch so I think this is not related to it. |
|
Read the documentation that is a good start. |
|
|
Let me take your questions one by one. I will take a look first at your test case, then at the production code. |
| return False | ||
| if self.registers[i].type in check: | ||
| continue | ||
| if self.registers[i].type is CellType.NEXT: |
There was a problem hiding this comment.
This is not right: real_address + 0 may not be NEXT, because it means you read off base.
For BITS, UINT16 next is never allowed (they occupy exact 1 register).
For UINT32, FLOAT32, first address must never be NEXT, second address must be NEXT, 3 address never etc....
For STRING first address must be STRING, next addresses must be NEXT / STRING.
So it is a bit more complex.
There was a problem hiding this comment.
As I said before, I don't really understand the NEXT celltype.
Why should it not be authorized to read the UINT32 in two requests or to read two UINT32 values in one request, but it's authorized to read only one register of the UINT32.
For strings it's similar, if there is a longer string and I would like to get only a part of it, why this wouldn't be allowed?
Initially I thought the type check is more of a function code check.
There was a problem hiding this comment.
NEXT is quite easy, it is the second register in UINT32, FLOAT32, and remaining registers in STRING.
Consider this sequence of registers:
12: UINT32, 13: NEXT, 14: UINT32, 15: NEXT, 16:UINT32, 17:NEXT
The following are valid reads:
Read_holding(12, 2)
Read_holding(12, 6)
Read_holding(14,2)
The following are invalid (Because they do not read whole UINT32)
Read_holding(12, 1)
Read_holding(13, 1)
Read_holding(12, 3)
FLOAT32 are exactly the same.
STRINGS are not so different:
12: STRING, 13: NEXT, 14: NEXT, 15: NEXT, 16: STRING, 17: NEXT
The following are valid:
read_holding(12, 4)
read_holding(12, 6) # read 2 complete strings (allowed just as it is allowed to read multiple UINT32)
read_holding(16, 2)
Type check is there to ensure you read the strings correctly, so the following is not allowed:
read_holding(12, 1)
read_holding(12, 1)
read_holding(13, 1)
Does that help ?
There was a problem hiding this comment.
The type check is there to ensure you read the registers as the designer of the device intended. It is a "verification", and you can find X reasons why it should be different, but then the answer is configure it to False!
|
When changing validate_type, we should also check that count for UINT32, FLOAT32 is a multiple of 2. |
|
Thanks for your reposes.
|
|
that is because you have something running that blocks port 5030. |
|
|
Look at CI, we use Linux and it works. |
janiversen
force-pushed
the
fix_simulator_01
branch
from
081c77e
to
54b7e03
Compare
|
Thanks. |
…ymodbus-dev#1323) Expand unit test to check for strings larger than 6 bytes (3 modbus registers) Co-authored-by: Georg Hofmann <georg@hofmannsweb.com> Co-authored-by: jan Iversen <jancasacondor@gmail.com>
This commit fixes the register type check for configuration where more than 3 registers are configured as one area (I think for the moment this happens only for string types for the moment).
I updated unit tests for this case.