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

[docs] Make clear that memory must be allocated #167

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@lagagja

lagagja commented Nov 22, 2013

Memory must be allocated when using the report slave id function, this is
not handled by the function. To be safe when the length of the response is
unknown you can use the constant MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h.

Update documentation just to help people and to point to the useful constant.

[docs] Make clear that memory must be allocated
Memory must be allocated when using the report slave id function, this is
not handled by the function. To be safe when the length of the response is
unknown you can use the constant MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h.

Update documentation just to help people and to point to the useful constant.
@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Nov 22, 2013

Owner

It's not possible to know how much memory is required so the API must be changed to add the max size in argument....

Owner

stephane commented Nov 22, 2013

It's not possible to know how much memory is required so the API must be changed to add the max size in argument....

@ghost ghost assigned stephane Nov 22, 2013

@lagagja

This comment has been minimized.

Show comment
Hide comment
@lagagja

lagagja Nov 22, 2013

Hi Stephane.
I'm afraid, I don't understand your comment.
I was suggesting to add a hint in the documentation, that the caller of the function is responsible for memory allocation - like the hint in the documentation of the function modbus_read_input_registers (similar the caller there is responsible for memory allocation).
The maximum length of the response is given by the constant MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h, so in the worst case the caller may use this constant, if he doesn't know anything about the expected response length.
I was not suggesting to change the API of the function modbus_report_slave_id.
We are actually using the function modbus_report_slave_id in the very same way as the committed example - at least we understood it in that way. Is this the wrong way to use the function?
I'm sorry, if the description of the pull request was confusing.

lagagja commented Nov 22, 2013

Hi Stephane.
I'm afraid, I don't understand your comment.
I was suggesting to add a hint in the documentation, that the caller of the function is responsible for memory allocation - like the hint in the documentation of the function modbus_read_input_registers (similar the caller there is responsible for memory allocation).
The maximum length of the response is given by the constant MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h, so in the worst case the caller may use this constant, if he doesn't know anything about the expected response length.
I was not suggesting to change the API of the function modbus_report_slave_id.
We are actually using the function modbus_report_slave_id in the very same way as the committed example - at least we understood it in that way. Is this the wrong way to use the function?
I'm sorry, if the description of the pull request was confusing.

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Nov 22, 2013

Owner

Le 22 nov. 2013 16:32, "Thomas Hilbig" notifications@github.com a écrit :

Hi Stephane.
I'm afraid, I don't understand your comment.
I was suggesting to add a hint in the documentation, that the caller of
the function is responsible for memory allocation - like the hint in the
documentation of the function modbus_read_input_registers (similar the
caller there is responsible for memory allocation).

When you use this function you can't know how manu bytes will be returned.

The maximum length of the response is given by the constant
MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h, so in the worst case the caller may use this
constant, if he doesn't know anything about the expected response length.

That's not the purpose of this constant and the constant must be backend
independent so I could add a new constant, a sort of max useful message
length or as previously said change the API. The user could give in
argument the max size it wants to receive (allocated size).

I was not suggesting to change the API of the function
modbus_report_slave_id.
We are actually using the function modbus_report_slave_id in the very
same way as the committed example - at least we understood it in that way.
Is this the wrong way to use the function?
I'm sorry, if the description of the pull request was confusing.

You use it the right way but the current API has deficiencies.


Reply to this email directly or view it on GitHub.

Owner

stephane commented Nov 22, 2013

Le 22 nov. 2013 16:32, "Thomas Hilbig" notifications@github.com a écrit :

Hi Stephane.
I'm afraid, I don't understand your comment.
I was suggesting to add a hint in the documentation, that the caller of
the function is responsible for memory allocation - like the hint in the
documentation of the function modbus_read_input_registers (similar the
caller there is responsible for memory allocation).

When you use this function you can't know how manu bytes will be returned.

The maximum length of the response is given by the constant
MODBUS_RTU_MAX_ADU_LENGTH
defined in modbus-rtu.h, so in the worst case the caller may use this
constant, if he doesn't know anything about the expected response length.

That's not the purpose of this constant and the constant must be backend
independent so I could add a new constant, a sort of max useful message
length or as previously said change the API. The user could give in
argument the max size it wants to receive (allocated size).

I was not suggesting to change the API of the function
modbus_report_slave_id.
We are actually using the function modbus_report_slave_id in the very
same way as the committed example - at least we understood it in that way.
Is this the wrong way to use the function?
I'm sorry, if the description of the pull request was confusing.

You use it the right way but the current API has deficiencies.


Reply to this email directly or view it on GitHub.

@lagagja

This comment has been minimized.

Show comment
Hide comment
@lagagja

lagagja Nov 25, 2013

Hi Stephane
Thanks a lot for your explanation. After studying the spec again, your source code and discussion with my colleague we figured out that the maximum number of bytes the function report_slave_id might return is actually 253, that is the Modbus PDU.
I think it would help people as a starter if you could add to the documentation of this function

  • You must take care to allocate enough memory to store the results in 'dest'.
  • The maximum number of bytes returned is 253

Your suggestion to introduce a new constant is great, like "MODBUS_PDU 253" or so, this one could be used to
create a safe destination array like
uint8_t tab_bytes[MODBUS_PDU];

Thanks again for your reply.

lagagja commented Nov 25, 2013

Hi Stephane
Thanks a lot for your explanation. After studying the spec again, your source code and discussion with my colleague we figured out that the maximum number of bytes the function report_slave_id might return is actually 253, that is the Modbus PDU.
I think it would help people as a starter if you could add to the documentation of this function

  • You must take care to allocate enough memory to store the results in 'dest'.
  • The maximum number of bytes returned is 253

Your suggestion to introduce a new constant is great, like "MODBUS_PDU 253" or so, this one could be used to
create a safe destination array like
uint8_t tab_bytes[MODBUS_PDU];

Thanks again for your reply.

@stephane stephane closed this in 52a82f8 Nov 25, 2013

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Nov 25, 2013

Owner

What do you think about my commit?

Owner

stephane commented Nov 25, 2013

What do you think about my commit?

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Nov 26, 2013

Contributor

Looks very good to me, but it would still be nice to get a define for the modbus PDU size of 253. The existing RTU or TCP size defines (256/260) work but are a) misleading, as you said :) and b) bigger than they need to be.

With this define, the report slave id example can be:

uint8_t tab_bytes[MODBUS_PDU_SIZE];
rc = modbus_report_slave_id(ctx, MODBUS_PDU_SIZE, tab_bytes);
if (rc > 1) {
      printf("Run Status Indicator: %s\n", tab_bytes[1] ? "ON" : "OFF");
}

Not everyone likes malloc ;)

Contributor

karlp commented Nov 26, 2013

Looks very good to me, but it would still be nice to get a define for the modbus PDU size of 253. The existing RTU or TCP size defines (256/260) work but are a) misleading, as you said :) and b) bigger than they need to be.

With this define, the report slave id example can be:

uint8_t tab_bytes[MODBUS_PDU_SIZE];
rc = modbus_report_slave_id(ctx, MODBUS_PDU_SIZE, tab_bytes);
if (rc > 1) {
      printf("Run Status Indicator: %s\n", tab_bytes[1] ? "ON" : "OFF");
}

Not everyone likes malloc ;)

@lagagja

This comment has been minimized.

Show comment
Hide comment
@lagagja

lagagja Nov 26, 2013

Hi Stephane.

Looks great, I'm impressed. Very logical and beautiful consistent with the other existing functions!
Thanks a lot.

I agree with Karl - would be nice to have such a constant.

lagagja commented Nov 26, 2013

Hi Stephane.

Looks great, I'm impressed. Very logical and beautiful consistent with the other existing functions!
Thanks a lot.

I agree with Karl - would be nice to have such a constant.

@stephane

This comment has been minimized.

Show comment
Hide comment
@stephane

stephane Nov 26, 2013

Owner

Thank you @karlp @lagagja, constant added.

Owner

stephane commented Nov 26, 2013

Thank you @karlp @lagagja, constant added.

mk8 added a commit to mk8/libmodbus that referenced this pull request Jan 29, 2014

mk8 added a commit to mk8/libmodbus that referenced this pull request Jan 29, 2014

@stephane

This comment has been minimized.

Show comment
Hide comment

stephane commented on 56b2526 May 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment