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.

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

AP_Button: range check PWM input #20070

Merged
merged 4 commits into from
Feb 28, 2022
Merged

AP_Button: range check PWM input #20070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b042e67
AP_Button: trigger low on invalid PWM input
IamPete1 Feb 9, 2022
1d92e8f
Copter: motor_test: use PWM min and max from RC_Channel
IamPete1 Feb 15, 2022
661ff77
Plane: motor_test: use PWM min and max from RC_Channel
IamPete1 Feb 15, 2022
6ca1829
RC_Channel: update RC_MIN_LIMIT_PWM from 900 to 800
IamPete1 Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ArduCopter/motor_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
*/

// motor test definitions
#define MOTOR_TEST_PWM_MIN 800 // min pwm value accepted by the test
#define MOTOR_TEST_PWM_MAX 2200 // max pwm value accepted by the test
#define MOTOR_TEST_TIMEOUT_SEC 600 // max timeout is 10 minutes (600 seconds)

static uint32_t motor_test_start_ms; // system time the motor test began
Expand Down Expand Up @@ -84,7 +82,7 @@ void Copter::motor_test_output()
}

// sanity check throttle values
if (pwm >= MOTOR_TEST_PWM_MIN && pwm <= MOTOR_TEST_PWM_MAX ) {
if (pwm >= RC_Channel::RC_MIN_LIMIT_PWM && pwm <= RC_Channel::RC_MAX_LIMIT_PWM) {
// turn on motor to specified pwm value
motors->output_test_seq(motor_test_seq, pwm);
} else {
Expand Down
4 changes: 1 addition & 3 deletions ArduPlane/motor_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
*/

// motor test definitions
#define MOTOR_TEST_PWM_MIN 800 // min pwm value accepted by the test
#define MOTOR_TEST_PWM_MAX 2200 // max pwm value accepted by the test
#define MOTOR_TEST_TIMEOUT_MS_MAX 30000 // max timeout is 30 seconds
Copy link
Member Author

Choose a reason for hiding this comment

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

interesting that this time out is different vs copters timeout.


// motor_test_output - checks for timeout and sends updates to motors objects
Expand Down Expand Up @@ -68,7 +66,7 @@ void QuadPlane::motor_test_output()
}

// sanity check throttle values
if (pwm >= MOTOR_TEST_PWM_MIN && pwm <= MOTOR_TEST_PWM_MAX ) {
if (pwm >= RC_Channel::RC_MIN_LIMIT_PWM && pwm <= RC_Channel::RC_MAX_LIMIT_PWM) {
Copy link
Contributor

@rmackay9 rmackay9 Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd just like to point out here that we are arguably mixing up pwm inputs with output...

// turn on motor to specified pwm vlaue
motors->output_test_seq(motor_test.seq, pwm);
} else {
Expand Down
15 changes: 11 additions & 4 deletions libraries/AP_Button/AP_Button.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,27 +74,27 @@ const AP_Param::GroupInfo AP_Button::var_info[] = {

// @Param: OPTIONS1
// @DisplayName: Button Pin 1 Options
// @Description: Options for Pin 1. PWM input detects PWM above or below 1800/1200us instead of logic level. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @Description: Options for Pin 1. PWM input detects PWM above or below 1800/1200us instead of logic level. If PWM is not detected or is less than 800us or above 2200us the button will interpreted as low. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @User: Standard
// @Bitmask: 0:PWM Input,1:InvertInput
AP_GROUPINFO("OPTIONS1", 6, AP_Button, options[0], 0),

// @Param: OPTIONS2
// @DisplayName: Button Pin 2 Options
// @Description: Options for Pin 2. PWM input detects PWM above or below 1800/1200us instead of logic level. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @Description: Options for Pin 2. PWM input detects PWM above or below 1800/1200us instead of logic level. If PWM is not detected or is less than 800us or above 2200us the button will interpreted as low. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @User: Standard
// @Bitmask: 0:PWM Input,1:InvertInput
AP_GROUPINFO("OPTIONS2", 7, AP_Button, options[1], 0),

// @Param: OPTIONS3
// @DisplayName: Button Pin 3 Options
// @Description: Options for Pin 3. PWM input detects PWM above or below 1800/1200us instead of logic level. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @Description: Options for Pin 3. PWM input detects PWM above or below 1800/1200us instead of logic level. If PWM is not detected or is less than 800us or above 2200us the button will interpreted as low. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @Bitmask: 0:PWM Input,1:InvertInput
AP_GROUPINFO("OPTIONS3", 8, AP_Button, options[2], 0),

// @Param: OPTIONS4
// @DisplayName: Button Pin 4 Options
// @Description: Options for Pin 4. PWM input detects PWM above or below 1800/1200us instead of logic level. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @Description: Options for Pin 4. PWM input detects PWM above or below 1800/1200us instead of logic level. If PWM is not detected or is less than 800us or above 2200us the button will interpreted as low. Invert changes HIGH state to be logic low voltage on pin, or below 1200us, if PWM input.
// @User: Standard
// @Bitmask: 0:PWM Input,1:InvertInput
AP_GROUPINFO("OPTIONS4", 9, AP_Button, options[3], 0),
Expand Down Expand Up @@ -183,6 +183,13 @@ void AP_Button::update(void)
continue;
}
const uint16_t pwm_us = pwm_pin_source[i].get_pwm_us();
if (pwm_us < RC_Channel::RC_MIN_LIMIT_PWM || pwm_us > RC_Channel::RC_MAX_LIMIT_PWM) {
// invalid pulse width, trigger low
if (pwm_state & mask) {
new_pwm_state &= ~mask;
}
continue;
}
// these values are the same as used in RC_Channel:
if (pwm_state & mask) {
// currently asserted; check to see if we should de-assert
Expand Down
2 changes: 1 addition & 1 deletion libraries/RC_Channel/RC_Channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ class RC_Channel {
const char *string_for_aux_function(AUX_FUNC function) const;
#endif
// pwm value under which we consider that Radio value is invalid
static const uint16_t RC_MIN_LIMIT_PWM = 900;
static const uint16_t RC_MIN_LIMIT_PWM = 800;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks OK but I can't resist saying that this constant's name should really be, "RC_PWM_LIMIT_MIN". Wouldn't that be better? Anyway, no need to change it I just want to pass on my naming wisdom.

Copy link
Contributor

Choose a reason for hiding this comment

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

LIMIT is kind of inferred by MIN :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, removing the LIMIT part would also be a fine change.. but not today I'm sure..

// pwm value above which we consider that Radio value is invalid
static const uint16_t RC_MAX_LIMIT_PWM = 2200;

Expand Down