Skip to content

Conversation

@jpmeijers
Copy link
Collaborator

Prevent an infinite loop in the readLine function. An infinite loop might occur when the wrong Serial port is defined. Therefore print out an error in the showStatus function, returning after 10 seconds, rather than waiting indefinitely.

@jpmeijers jpmeijers requested a review from johanstokking June 14, 2017 11:15
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Remove comments and format the code (spaces between operators, use code formatter)

Also maybe while (!read && loops--) is short and clear?

@jpmeijers
Copy link
Collaborator Author

while (!read && loops--) is indeed shorter, but I am not a fan of that style. I prefer to write longer code, which is more clear and less prone to bugs. In the end the compiler will anyway optimise it out.

while (loops--) is really difficult to read for someone with no or little C experience. I therefore agree with: #205 (comment)

But this comes down to style preference. If you want the library to follow your style I'll change it.

{
readResponse(SYS_TABLE, SYS_TABLE, SYS_GET_HWEUI, buffer, sizeof(buffer));
debugPrintIndex(SHOW_EUI, buffer);
if(strlen(buffer)<16)
Copy link
Member

Choose a reason for hiding this comment

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

Formatting

@johanstokking johanstokking force-pushed the feature/prevent-inf-loop branch from a3d99b3 to ada96c1 Compare June 22, 2017 12:51
johanstokking
johanstokking previously approved these changes Jun 22, 2017
while (read == 0 && timeout > 0)
while (read == 0 && attempts > 0)
{
timeout--;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

readBytesUntil has a timeout of 1000ms. If you have multiple attempts to call that function, the attempts are directly proportional to the timeout. timeout makes more sense to me, because it is a value in seconds.

@johanstokking johanstokking dismissed their stale review June 22, 2017 15:22

This breaks with default retry count

@jpmeijers jpmeijers closed this Jun 22, 2017
@johanstokking
Copy link
Member

This should only be used after resetting the module, to test that there is actually a module out there.

Otherwise, the RN module is stable enough to respond with something. Setting an arbitrary timeout, even user defined, is causing trouble by having stuff available on the serial that causes issues with request/response expectations.

@jpmeijers
Copy link
Collaborator Author

The original reason I added this was to notify the user that communication with the RN was unsuccessful. This can be either because the wrong serial port was defined and passed to this library, or that the RN module is not responding (bad wiring, etc). In the current state the library will just hang indefinitely.

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