Skip to content
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

SRV_Channel: use enumeration for servo input type #21396

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

peterbarker
Copy link
Contributor

No description provided.

Comment on lines +53 to +44
AUX = 16,
ANGLE = 17,
RANGE = 18,
Copy link
Member

Choose a reason for hiding this comment

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

Why start at 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better than zero :-) I'd usually wouldn't choose 16, but on this day I did. Since it's an internally-used enumeration the values don't matter, and if you're stepping through gdb and find the number 17 it's a lot easier to search for that value in the relevant header vs 0 or 1.

@@ -49,6 +49,12 @@ class SRV_Channel {
public:
friend class SRV_Channels;

enum class Type {
AUX = 16,
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be RANGE=0 to be sure that the default is the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this by setting the value in the constructor to Type::Range. Changing the enumeration values here wouldn't have helped.

@peterbarker peterbarker force-pushed the pr/srv-channel-type-enum branch 3 times, most recently from 094e932 to 5bb4ac1 Compare August 19, 2022 09:57
Copy link
Contributor

@magicrub magicrub left a comment

Choose a reason for hiding this comment

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

So, what output type is aux output?

@peterbarker
Copy link
Contributor Author

So, what output type is aux output?

Comes down to "whatever the library responsible for the pin wants it to be". For example, NeoPixel does digital stuff on the pins it owns (k_LED_neopixel1 etc) - IOW it isn't even doing pwm on those pins. It's up to the handling code to set the range/angle if they want to do PWM output and specify the output level using a range or angle value. If you look at AP_Gripper_Servo.cpp you'll see that it sets the pwm output level directly.

Breaking this out into a distinct enumeration value is meant to help catch where we set things with the angle calls but haven't specified a range for the angles...

@peterbarker peterbarker force-pushed the pr/srv-channel-type-enum branch from 5bb4ac1 to e1f1958 Compare August 22, 2022 02:30
@peterbarker peterbarker force-pushed the pr/srv-channel-type-enum branch 2 times, most recently from 98f03dd to ee3d9cf Compare May 15, 2023 06:09
@peterbarker peterbarker force-pushed the pr/srv-channel-type-enum branch from ee3d9cf to 061220e Compare February 5, 2025 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

5 participants