-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
subsys:modbus: Atomic read / write for multiple registers access FCs: 0x03, 0x04, 0x010 #73237
Comments
Well, you have to deal with a "partial" or "unaligned" access anyway, so I really do not see much benefit in any of the proposed changes. |
How do you mean it - "either partial or unaligned" or "partial and unaligned". I feel the benefits are quite obvious:
I have dealt with with both approaches using a commercial Modbus stack (from a well known debugger vendor) which also basically allowed the proposed 3. solution (doing a direct custom/user solution for every FC). From experience I can say, it is by far easier than fiddling with multiple callback calls. I feel at least the freedom to do a custom implementation for all FCs should be given (3. solution proposal). |
Logical
I do not see the cost of calling a callback to be that high, especially if it is optimized away.
No idea what you mean.
It sounds like you should rethink your approach to validating and overwriting the data. Perhaps it would help if the Modbus server implementation had lock/unlock callbacks that are called at the beginning/end of the command processing?
Well, it is intentional that the callback arguments are kept to a minimum to minimize possible application errors, so the user would not even get the idea of doing memcpy with buffer of Modbus subsystem. This will not change.
No, mandatory FCs should comply with the spec and should not be overloadable.
That is okay, but it does not convince me.
No, what you think you can save on the code is not worth the additional Kconfig options to maintain. |
It is not about the exec time of calling a callback and that it has to be called several time. It is about slicing what ever data block I want to transmit (e.g. a time/date format which does not fit in 16 bits) into 16 bit chunks and iterate over those N times instead of just getting the pointer to the buffer an copy the time/date format aligned as required.
Example: again writing time / date to the device:
Is simply does not make sense, that the stack slices the data block into 16 bit chunk just for the callback to puzzle it together again.
I do not think it is a com stacks task to lock or unlock something in the FW. And it will not help if the last call to the callback has to call an FW internal API setting the date but the com stack keeps that locked until returning from the last call to the callback.
I cannot see why the user should not be allowed to copy the payload. Just because of bad callback implementations would copy too much or overwrite something in the tx buffer? Then one has to pass everything by value and never by ref / pointer.
Without have read the spec in any detail, I doubt that the spec specifies the impl. to loop over the number of registers. This is impl. detail.
If you only have 64kB Flash, 100 Byte here and there can make the difference. |
I am facing this issue right now. IMO FC16 is the only critical issue right now, block writes should be able to be handled in application code as blocks. Multi-reg read requests are easier to handle. With the existing implementation, receiving any FC16 means that I need some intermediate data storage between the modbus and the final location, and to have to use indeterminate timeouts/assumptions in order to trigger write done. I support approach 2 which would be a simple change in implementation
|
Why should reading be simpler? Also with reading one needs to fetch the data block at once for consistency of the data in the block, store it somewhere in an intermediate storage and then iterate over it as 16bit registers to be transferred and BE endianessed to the tx buffer. There even is a, IMO, non-standard float implementation starting at register number 5000 and above, if enabled. That implementation changes endianess over all 4 float bytes to BE. The 16 bit register impl. will change the float byte wise in the two words. On an little endian machine you get "float little-endian byte swap according to https://www.modbustools.com/poll_display_formats.html In the Modbus spec. nothing else but bit and word (16 bit) data types are defined. I still think it needs all three and the word byte swap as option for the implementation (presumably the appl.) that fills the tx buffer to have the freedom to decide which endianess shall be used on the bus for the payload (LE, BE, LE-byte swap, BE-byte swap) of multi word regs read and write are required. |
|
I meant more that at least reading is more doable with the existing implementation. Its easier to async read a slice of data then async write a slice, but agree that block writes for both should be supported
Agree 100%. I'm in favour of the additional user callbacks, as opposed to FC overloads. With standard FC, I just want to deal with the incoming/outgoing data, not worry about the response values etc. |
Introduction
Reading and writing multiple IRs/HRs is currently done by iterating over the register to be read/written.
The respective user callback only pass the address and the value resp. value pointer and is called multiple times.
In order to be able to access e.g. a date / time, a 32 or 64 bit values or even struct like data atomically, iteratively accessing multiple 16 bit values is complicated/elaborated.
Problem description
Accessing multiple 16 bit registers to read 32, 64 bit or even struct-, record-like data in an atomic manner requires unnecessary puzzling together of 16 bit fragments due the iterative multiple calls of the user/application callbacks.
If the the callback would be called once passing the address, a value pointer and a number of regs to be read or written, the callback can check for the number of expected regs, read fetch the data from the firmware (read) or write the data to the firmware (write) and return.
Proposed change
1. Solution
Replace the existing
in
struct modbus_user_callbacks
with an new signatureConcern: This change will create an backward incompatible interface
2. Solution
Add new callbacks
in
struct modbus_user_callbacks
with an new signaturestruct modbus_user_callbacks
and check for NULL pointers in the code.Concerns:
1. One can have only either or
2. More flexible, but more code
3. Solution
Move
send_reply = mbs_try_user_fc(ctx, fc);
before the switch case checking the standard function codes.
This allows to 'overload' the standard implementation by a user/custom implementation.
mbs_try_user_fc(ctx, fc)
then needs to returnMODBUS_EXC_ILLEGAL_FC
if the FC wasnot implemented as user/custom solution. Only then the switch with the standard implementation
will be executed.
In case the FC was found by
mbs_try_user_fc(ctx, fc)
(FC was implemented by the user), it returnsMODBUS_EXC_NONE
plus aMODBUS_SEND_REPLY_FLAG
At least the return value of
mbs_try_user_fc(ctx, fc)
will become rather an int then a bool.Concern: Multiple IR/HR read / write with single callback call will always have to be impl. as user/custom solution and is not provided by Zephyr
4. Solution
Concern: Mitigates concern of 3. solution but also opening up for a 3. implementation besides 1. and 2.
Preferred solution
Our preferred solution is 2.1 (add new interface switchable with KConfig) or 3. (Overwrite standard impl. by user).
If we impl. 2.1 we will very like no longer impl. 3. too, since we do not need both.
Detailed RFC
Pls see chapter 'Proposed change'
Proposed change (Detailed)
Pls see chapter 'Proposed change'
Dependencies
Concerns and Unresolved Questions
Pls see concerns in proposed solutions
Alternatives
Pls see solution proposals.
The text was updated successfully, but these errors were encountered: