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

Make internal buffer size configurable #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lammermann
Copy link

I use this library in a constraint embedded environment, where I need a smaller buffer size to run it.
Would you consider accepting this change to make the buffer size configurable by the user?

Greetings Benjamin

@debevv
Copy link
Owner

debevv commented Mar 10, 2023

Hi, thanks for your contribution. The are some things you should check:

  • The code in the PR does not build. There should be a typo in the define name
  • NMBS_INTERAL_MAX_REGISTER_QUANTITY size may not be correct for both TCP and RTU transports. Please check the standard
  • Please format the code using the provided .clang-tidy file

@lammermann
Copy link
Author

Hi, thanks for your contribution. The are some things you should check:

* The code in the PR does not build. There should be a typo in the define name

Sorry that I was so sloppy with the first patch. I fixed the typo now.

* `NMBS_INTERAL_MAX_REGISTER_QUANTITY` size may not be correct for both TCP and RTU transports. Please check the standard

I tried to fix it according to the modbus spec. As modbus tcp needs more bytes than rtu I used it's size to be safe in both cases. However I recognized that some tests are failing now as they check that a quantity bigger than 125 fails with an error message. What is the reason for the hard coded 125 limit?
I see two solutions here: Either reducing the default value of NMBS_INTERNAL_BUFFER_SIZE to 257. Or to change the tests. What do you prefer?

* Please format the code using the provided .clang-tidy file

I ran clang-tidy nanomodbus.c nanomodbus.h but it did not reformat anything. The only complaints it had were about using memset but those are not connected to the code I changed. I never used clang-tidy before so it's possible I did not use it correctly.

Greetings, Benjamin

@lammermann lammermann force-pushed the master branch 2 times, most recently from 98acdbb to 4b1d5a3 Compare August 14, 2023 10:27
spell check on NMBS_INTERNAL_.........
spell check NMBS_INTERNAL_.....
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants