Skip to content

Conversation

@MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Feb 17, 2021

Describe problem/solution in this pull request
New ManualControl class for handling manual_control_setpoint (and later switches) holding subscription, parameters and state necessary. It handles in this pr RC loss, RC override, RC arming/disarming.

  • RC override: option to ignore throttle stick
  • RC override: based on change (not centered stick)
  • RC arming: based on Hysteresis instead of "loop ticker"
  • RC arming: simplify the logic
  • manual_control_setpoint message

Note: This is the first stab at it and I'd like to do incremental steps to not create a non-understandable monster pr.

Test data / coverage
I tested the different cases during development but will do a final pass testing all the things with this state.

Additional context
Follow up after being in the loop from reviewing #16266 and doing work on RC override.

@MaEtUgR MaEtUgR requested a review from dagar February 17, 2021 19:16
@MaEtUgR MaEtUgR self-assigned this Feb 17, 2021
@MaEtUgR MaEtUgR force-pushed the throttle-stick-override-upstream branch from 85b3d44 to 98c7c61 Compare February 17, 2021 22:38
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 22, 2021

As a response to @dagar 's feedback:

looks like a good direction
my only initial suggestion/question is if we can get to the point where it's more event driven so that as much code as possible is only ever executed as a result of manual_control_setpoint updating

I prepared to gate all this processing on manual_control_setpoint updates:
throttle-stick-override-upstream...rc-handling
Testing necessary but then we can add it as well.

@MaEtUgR MaEtUgR changed the title Commander: Separate out manual control setpoint processing [WIP] Commander: Separate out manual control setpoint processing Feb 24, 2021
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 24, 2021

Arming/disarming not working as expected on a vehicle without props. Debugging.

@MaEtUgR MaEtUgR force-pushed the throttle-stick-override-upstream branch from 98c7c61 to 9226e30 Compare February 24, 2021 15:49
@MaEtUgR MaEtUgR changed the title [WIP] Commander: Separate out manual control setpoint processing Commander: Separate out manual control setpoint processing Feb 24, 2021
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 24, 2021

I found the issue: ebbc098
Ready for review. I'll keep on testing and will only put bug fixes on top if necessary. Arming, disarming using all methods works.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 24, 2021

Found another issue: Real-world RC seems to bounce RC loss, not reproducible on master or in SITL.
image

@MaEtUgR MaEtUgR force-pushed the throttle-stick-override-upstream branch from ebbc098 to abe5135 Compare February 25, 2021 13:14
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Feb 25, 2021

Fixed, squashed and rebased.

Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Couple of comments but overall that's a very nice cleanup, exactly the sort of thing we should do step by step.


using namespace time_literals;

enum OverrideBits {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not enum class?

Copy link
Member Author

@MaEtUgR MaEtUgR Mar 3, 2021

Choose a reason for hiding this comment

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

It gets used in combination with a "bitwise and" not in a direct comparison because the parameter is a bitfield. Would you define the bitwise and operator in the class or how can it be done more easily?

That's probably also why it was added in this way:
https://github.com/PX4/PX4-Autopilot/pull/13618/files#diff-33729b6612e70db2803e6b18b64a47c71e70eaf1d6b69fd233a34b3b0408a806R261

* Check for manual control input changes and process them
* @return true if there was new data
*/
bool update();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super happy with the name update() for something that also returns something but I also don't have a better suggestion, unless maybe couldUpdate() or tryUpdate()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this because I also don't like if everything is called update() for e.g. text search reasons. But updated() is wrong because it actually does something and I couldn't come up with something better. Maybe checkUpdateAndProcess()? Seems clunky and doesn't really help does it?

@MaEtUgR MaEtUgR force-pushed the throttle-stick-override-upstream branch from 7789288 to d063977 Compare March 3, 2021 17:56
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes @MaEtUgR!

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Mar 4, 2021

Thank you for the reviews @julianoes and @bresch and @JonasVautherin, that really helped a lot. I'll follow up with further improvements like freeing commander from the rc_found_once logic, doing all the processing in the process() function, separating arming methods.

@MaEtUgR MaEtUgR merged commit 5bbc66f into master Mar 4, 2021
@MaEtUgR MaEtUgR deleted the throttle-stick-override-upstream branch March 4, 2021 09:41
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.

6 participants