Skip to content

Remove extra null, only if length is one byte longer then message.#7

Merged
yaacov merged 5 commits intomasterfrom
fix-length-check
Oct 31, 2017
Merged

Remove extra null, only if length is one byte longer then message.#7
yaacov merged 5 commits intomasterfrom
fix-length-check

Conversation

@yaacov
Copy link
Copy Markdown
Owner

@yaacov yaacov commented Oct 30, 2017

Remove extra null, only if length is one byte longer then message.

In #6 we remove the extra null when length is bigger then 8, this is a problem for valid messages of length 11 and 13.

In this pull request we look for specific length that can have an extra null.

@raffaeler please take a look

Comment thread src/ModbusSlave.cpp Outdated
* is get from the SoftwareSerial on the "nano"
* posible message length are:
* FC1..6 : length = 8 (with extra null 9)
* FC15 : length = 11 (with extra null 12)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo, this is very wrong ... will fix :-(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will need to do crc after checking command and length ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like hardcoding the length (as I also did in the first pull request).
If anybody will ever implement new messages that are part of the modbus standard, it will be a mess again.
I would prefer to isolate a function calculating the message length and trimming the nulls at the end.
Do you agree?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yes, fixed that ...

Comment thread src/ModbusSlave.cpp Outdated
* Get message address and length/status.
*/
address = word(bufIn[2], bufIn[3]); // first register.
length = word(bufIn[4], bufIn[5]); // number of registers to act apone.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

act upon or status

Comment thread src/ModbusSlave.cpp Outdated
length = word(bufIn[4], bufIn[5]); // number of registers to act apone.

/**
* this removes any trailing noise after message
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanity check and remove ...

@yaacov
Copy link
Copy Markdown
Owner Author

yaacov commented Oct 30, 2017

@raffaeler can you check that this version works for you ?

@raffaeler
Copy link
Copy Markdown
Contributor

Yes, it works on my sketch, but I will also quickly check the code since my sketch does not use all the functions

@raffaeler
Copy link
Copy Markdown
Contributor

I am still working on it, sorry for the previous, deleted msg

@yaacov
Copy link
Copy Markdown
Owner Author

yaacov commented Oct 30, 2017

np :-)

@raffaeler
Copy link
Copy Markdown
Contributor

There could be a problem when calculating the length.
If the length is 8 (such as in my case), no problem.
If the length is greater, and since this is calculated, you should check the buffer is large enough.
The sanity check is good: if (length > MAX_BUFFER) return 0;
But since lengthIn can be far larger: lengthIn = (int)(7 + length * 2 + 2);
there is the chance of a buffer overflow right after the switch/case.
What do you think?

@yaacov
Copy link
Copy Markdown
Owner Author

yaacov commented Oct 30, 2017

But since lengthIn can be far larger: lengthIn = (int)(7 + length * 2 + 2);
there is the chance of a buffer overflow right after the switch/case.
What do you think?

Right, thanks !

@yaacov
Copy link
Copy Markdown
Owner Author

yaacov commented Oct 30, 2017

@raffaeler Thanks, added the missing check.

@yaacov yaacov merged commit 8509993 into master Oct 31, 2017
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.

2 participants