-
Notifications
You must be signed in to change notification settings - Fork 47
add reset_input_buffer in AbstractCephlaMicroSerial class and SimSeri… #167
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
Conversation
ianohara
left a comment
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.
I think using the _update_internal_state() helper is more in line with the rest of the code, and easier for future maintenance, unless I missed a reason why we can't use that. Can you update it if there's not a reason?
software/control/microcontroller.py
Outdated
| return True | ||
| except Exception as e: | ||
| self.log.error(f"Failed to clear input buffer: {e}") | ||
| self._log.error(f"Failed to clear input buffer: {e}") |
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.
I didn't know about this initially, so I was doing the same for a while, but if you want you can use:
self._log.exception("Failed to clear input buffer")
And as long as you call that in an except clause, it'll take care of printing the exception (and stack trace!) for you.
software/control/microcontroller.py
Outdated
| def reset_input_buffer(self) -> bool: | ||
| with self._update_lock: | ||
| self.response_buffer.clear() | ||
| self._in_waiting = 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.
I think the convention here is to run self._update_internal_state() instead of modifying self._in_waiting directly. Can you do that instead? Or does that not work for some reason?
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.
(the idea of self._update_internal_state() is that it tries to keep all the mutating and calculate of fields in one place as much as possible, then all the methods can just call it and not need to know about the particulars of how to update all the internal state.
ianohara
left a comment
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.
Looks good to me!
…al class