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

rc_update: fix on-off-switch with negative threshold values #21796

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Jul 3, 2023

Solved Problem

When reviewing #21763 I wasn't exactly sure why the fix is needed at all see #21763 (comment). I tested on hardware and realized it's actually broken and I still don't know why it broke. But I was sure I wanted it to not break the same way again and the function to be more readable. So I added unit testing and fixed it by simplifying the function rather than adding to it.

Fixes #19809
Closes #21763

Solution

  • Improve unit testing of the RCUpdate class I added in rc_update: Add unit tests for mode switch and button #19579
    • adding protected scope again
    • setting parameters from outside the class
  • Add unit tests for the return switch
    • Positive threshold cases
    • Corner cases
    • Negative threshold cases that failed
  • Fix negative threshold cases

Changelog Entry

Bugfix: Negative RC switch thresholds

Test coverage

Unit tests that I added. I can also test on hardware if desired.

Channel values stay over one unit test but some tests assumed they are
reset each time. Reset the channel after these mode button tests.

Parameters survive between unit tests presumably as long as
the bianry runs. Reset them if a test requires that.
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jul 4, 2023

I made a minimal bugfix port for the 1.14 release: #21801
Could this pr get reviewed? Then we could merge both sides.

Copy link
Contributor

@junwoo091400 junwoo091400 left a comment

Choose a reason for hiding this comment

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

Seems nice, thanks for this follow-up PR! Let's get this in main, then into 1.14 🔥

src/modules/rc_update/RCUpdateTest.cpp Outdated Show resolved Hide resolved
src/modules/rc_update/rc_update.cpp Show resolved Hide resolved
src/modules/rc_update/RCUpdateTest.cpp Show resolved Hide resolved
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-community-q-a-july-05-2023/32963/1

Co-authored-by: Junwoo Hwang <junwoo091400@gmail.com>
@MaEtUgR MaEtUgR merged commit 2dcb525 into main Jul 11, 2023
81 of 85 checks passed
@MaEtUgR MaEtUgR deleted the maetugr/rc-update-test branch July 11, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Negative RC threshold not working in v1.13.0
3 participants