Skip to content

Refactoring: Separating definition/implementation and other minor cleanups #42

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 21, 2019
Merged

Refactoring: Separating definition/implementation and other minor cleanups #42

merged 7 commits into from
Mar 21, 2019

Conversation

aentinger
Copy link
Contributor

@aentinger aentinger commented Mar 20, 2019 •
edited
Loading

No change in logic/functionality of the implementation.

@aentinger aentinger requested a review from ubidefeo March 20, 2019 07:17
@aentinger aentinger marked this pull request as ready for review March 20, 2019 07:19
@ubidefeo
Copy link
Collaborator

hi @lxrobotics
I appreciate this effort a lot.
went through the code and it's cleaner, just want to test it a little today if you don't mind :)

@mattiabertorello mattiabertorello self-requested a review March 20, 2019 11:57
@aentinger
Copy link
Contributor Author

Hi @ubidefeo - please do so 👍 Let me know if you come across any issues 😃

@ubidefeo
Copy link
Collaborator

hey @lxrobotics
would it trouble you to add a way to specify which Serial port can be used for the debug messages?
I haven't been able to get around it and I could benefit from having this option :)

@aentinger
Copy link
Contributor Author

Sure, I can do that 👍 I'd prefer to do it in another pull request though and first merge and close this.

ubidefeo
ubidefeo approved these changes Mar 21, 2019
View reviewed changes
Copy link
Collaborator

@ubidefeo ubidefeo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

very happy with the changes.
I have also been testing this and it works well 👍

mattiabertorello
mattiabertorello approved these changes Mar 21, 2019
View reviewed changes
Copy link
Contributor

@mattiabertorello mattiabertorello left a comment

Choose a reason for hiding this comment

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

LGTM

@aentinger aentinger merged commit 4b9f851 into master Mar 21, 2019
@aentinger aentinger deleted the refactor branch March 21, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattiabertorello mattiabertorello mattiabertorello approved these changes

@ubidefeo ubidefeo ubidefeo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aentinger @ubidefeo @mattiabertorello