-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
manual control selection and cleanup #17404
Conversation
9f2d21c
to
ccf5da6
Compare
4ec1a44
to
24fb580
Compare
Note to self: I should go through commander and clean up some of the now redundant RC loss checking. |
Don't bother reviewing just yet. This needs more cleanup work on the commander side. |
Ok, now it's in a somewhat reviewable state, I hope. |
d37bbf1
to
9fb6266
Compare
6b5d266
to
033a0b9
Compare
Bump, please review. |
4a6f736
to
3aaa101
Compare
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'm on it but it's a bigger junk to review 🚚
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.
Some more comments, I'm not through yet 🐌
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 studied some more. Still not completely through sorry 🏃
3aaa101
to
4d5a711
Compare
@MaEtUgR thanks a lot for the in-depth review. I think I have addressed most points, except:
I've rebased the branch and force pushed, so to review the changes, best just check the recent commits. |
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.
Just adding this now already in the hope it's what prohibits me from answering in the existing conversations.
EDIT: It worked I just couldn't respond because there was a pending review 🤦
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.
Now I'm completely through 😅 I hope the feedback helps. I'm happy to cooperate e.g. on the arm button logic. I added two commits for the unit test I came up with that fails and to remove the commented-out lines.
My main concerns are:
- arming button "broken"
- gear switch doesn't cover all cases
- MAVLink command target IDs
@MaEtUgR I think I've answered to most of your points and fixed most of them. I'm also rebasing this on latest master, so to see if my fixes make sense, you can just check the latest commits. |
59115a6
to
9e3f4a2
Compare
f9f5842
to
39ec0d0
Compare
Final (?) rebase on |
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.
Jenkins fails on src/drivers/uavcan/libuavcan
and the targets build for me locally.
I'm done with my testing.
@julianoes @dagar Merge?
This adds a new module called manual_control which handles as much as possible of RC and MAVLink manual_control input outside of commander.