-
Notifications
You must be signed in to change notification settings - Fork 55
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
Fix timeout management in ROBUS protocol #484
Fix timeout management in ROBUS protocol #484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could remove the boolean parameter from your modified function. This would drastically reduce the number of modifications and improve code readability.
By modifying Robus this way you broke the compilation for all the other HAL. You should consider applying your modifications to the other HAL.
|
||
LL_TIM_SetAutoReload(ROBUS_TIMER, DEFAULT_TIMEOUT); | ||
LL_TIM_SetCounter(ROBUS_TIMER, 0); | ||
// HAL_GPIO_WritePin(GPIOA, GPIO_PIN_4, RESET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove those "test" comments.
RobusHAL_ResetTimeout(DEFAULT_TIMEOUT); | ||
RobusHAL_ResetTimeout(true, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case you want to enable the timer at DEFAULT_TIMEOUT. Instead of 0 you could use DEFAULT_TIMEOUT to improve readability.
(This would allow you to remove the boolean parameters, see other review comment)
RobusHAL_ResetTimeout(0); | ||
RobusHAL_ResetTimeout(false, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, you just want to shut down the timer so putting the value to 0 makes sense. You could remove the boolean value and just check if the value is 0.
d03b7f3
to
eb9ade5
Compare
By submiting this PR, you agree with the associated license MIT) and with our Contributor License Agreement (CLA).
Before to begin
Thank you for contributing to the Luos project!
Before to begin, please follow these steps:
Feel free to read the Luos contribution's guidelines and the documentation page to have more insight about how to contribute to Luos.
PR Description section
Description and dependencies
Please include here a summary of the changes and the related issue. List any dependencies that are required for this change.
Changes
Please choose the relevant options:
Related issue(s)
Provide a list of the related issues that will be fixed by this PR.
WARNING: Do not edit the checklist below.
Developer section
QA section
🆕 Feature: [Feature] Description...
🆕 Added: [Feature] Description...
🆕 Changed: [Feature] Description...
🛠️ Fix: [Feature] Description...