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

Freemodbus serial slave allow to access one extra register beyond data area (IDFGH-4768) #6571

Closed
LongRail opened this issue Feb 18, 2021 · 6 comments
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@LongRail
Copy link

If you defined area descriptor with size let's say 4 byte (2 modbus registers), the slave modbus will react without error
on requests up to 3 registers, returning unknown value for 3rd one.

I think the problem is in 'esp_modbus_slave.c: 66'
&& ((addr + regs) <= (it->start_offset + reg_size +1)) // <-- why +1 is here?

if removed, modbus slave works as expected.
It is checked in 0-based modbus addressing.

@github-actions github-actions bot changed the title Freemodbus serial slave allow to access one extra register beyond data area Freemodbus serial slave allow to access one extra register beyond data area (IDFGH-4768) Feb 18, 2021
@alisitsyn
Copy link
Collaborator

alisitsyn commented Feb 19, 2021

@LongRail,

Thank you for your issue.
This is mistake that was observed during testing but in the final MR was not fixed properly. This +1 in the line below

&& ((addr + regs) <= (it->start_offset + reg_size +1))

came from the stack which calls the callback function with address parameter already increased by one. It is better to fix it in mbc_slave_find_reg_descriptor() and have zero based register address in the callback function for the log. This will be fixed accordingly.

@LongRail
Copy link
Author

The line 66 of esp_modbus_slave.c is a body of mbc_slave_find_reg_descriptor(...) function, so does it mean the fix is in right place?
And when we can expect the official release with Modbus fixed?

@alisitsyn
Copy link
Collaborator

Yes, the fix above looks correct to me. The MR is actual and will be merged ASAP. I do not have any expectation when it will be merged.

@alisitsyn
Copy link
Collaborator

@LongRail,

The fix was merged in v4.3 commit # 60dfb09. This will be replicated into github repo once all CI tests pass on master.

@allard-potma
Copy link

allard-potma commented Mar 12, 2021

Any updates on this? The commit ID above cannot be found.
Meanwhile i've edited the line above to
&& ((addr + regs) <= (it->start_offset + reg_size)) should this be enough to fix this?
When I register a 4 byte value at modbus address 0x0000 and another 4 byte value at modbus address 0x0002, i am able to read the first and second 4 bytes when I read them separate, but not when I read all 8 bytes at once, which should be possible.

@alisitsyn
Copy link
Collaborator

The fix has been merged two weeks ago with commit #60dfb09122f079af4e513a87e3778a5de252e6ca.
I can just send the patch for you with this fix:
patch.txt

If I understood you correctly you register two continous areas and try to read them in one transaction. This case is not supported for now. You can define it as a whole one area instead if you need to read it in one transaction. The feature with definition of multiple modbus areas is designed to define several address spaces with the gaps between them.

@espressif-bot espressif-bot added Resolution: Done Issue is done internally Status: Done Issue is done internally labels Mar 18, 2021
espressif-bot pushed a commit to espressif/esp-modbus that referenced this issue Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

4 participants