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

Move test_tone and test_ppm out of test_hrt.cpp file into their own respective files. #11234

Merged
merged 3 commits into from
Feb 10, 2019
Merged

Move test_tone and test_ppm out of test_hrt.cpp file into their own respective files. #11234

merged 3 commits into from
Feb 10, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Jan 17, 2019 •
edited by AuterionWrikeBot
Loading

Hi there,

This PR places test_tone and test_ppm into their own files to follow the convention in all of the other src/systemcmds/tests/* files.

Let me know if you have any questions on this PR!

-Mark

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 2 times, most recently from 16d8887 to f47a39e Compare January 17, 2019 17:52
@mcsauder mcsauder mentioned this pull request Jan 17, 2019
Merged
@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch from f47a39e to 2c6f50a Compare January 19, 2019 01:28
@weekly-digest weekly-digest bot mentioned this pull request Jan 19, 2019
Closed
@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 4 times, most recently from 6954752 to 9110377 Compare January 21, 2019 22:40
davids5
davids5 requested changes Jan 22, 2019
View reviewed changes
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

Please make one pass and update the Copyright dates. Also there files have the old Nuttx section format (line 57-81) It would be OK to remove the empty comment blocks.

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch from 9110377 to 64659cf Compare January 22, 2019 16:46
@mcsauder mcsauder mentioned this pull request Jan 22, 2019
Merged
@mcsauder
Copy link
Contributor Author

Thanks @davids5! Done and pushed up. I finished out the licenses/copyrights in the rest of the files in the directory as well in PR #11273.

davids5
davids5 previously approved these changes Jan 22, 2019
View reviewed changes
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@mcsauder - Thank you!

@davids5 davids5 requested a review from dagar January 22, 2019 18:06
@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 5 times, most recently from 459732d to a682df5 Compare January 23, 2019 20:38
davids5
davids5 previously approved these changes Jan 23, 2019
View reviewed changes
src/systemcmds/tests/test_tone.cpp Outdated
int fd, result;
unsigned long tone;

fd = px4_open(TONEALARM0_DEVICE_PATH, O_WRONLY);
Copy link
Member

Choose a reason for hiding this comment

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

Are you going to remove ioctl on a subsequent PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After today's dev call I decided to add a second commit to this PR to take care of the IOCTL work, see comment and test results below.

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 7 times, most recently from f01e4b1 to 6305f82 Compare January 26, 2019 18:04
@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 2 times, most recently from 27bcc56 to ce83519 Compare January 31, 2019 00:21
@mcsauder
Copy link
Contributor Author

@dagar and @davids5 ,

The first commit of this PR formats tests_main.c/h and changes a for() loop index from unsigned to size_t, it does not modify the test_hrt, test_tone, or test_ppm tests; those tests are just broken out into separate files and some #includes deleted.

The second commit of this PR modifies the test_tone() unit test to use uORB rather than IOCTL() calls in line with the dev call discussion this morning.

The refactored test_tone() test no longer shows as failing and also produces an audible tone when run successfully, which was not the case previously.

Test results from this PR:
image

Test results from current Firmware/master, (test_tone failing):
image

davids5
davids5 previously approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@mcsauder - Thank you this looks good to go now.

@davids5
Copy link
Member

davids5 commented Jan 31, 2019

@mcsauder - I had a thought - use the tone API to indicate the success or failure of the tests. The would give an audible result to a tester?

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 5 times, most recently from 34569e2 to 2d98b47 Compare February 4, 2019 23:52
@mcsauder
Copy link
Contributor Author

mcsauder commented Feb 5, 2019

@davids5 , I've created the functionality we discussed. It's nice to get a tone when a test passes or fails. Let me know if you'd prefer a different tone for pass/fail. Thanks for the direction!

davids5
davids5 requested changes Feb 5, 2019
View reviewed changes
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@mcsauder - Thank you - This looks great see one comment

src/systemcmds/tests/tests_main.c Outdated
for (unsigned i = 0; tests[i].name; i++) {
if (!strcmp(tests[i].name, argv[1])) {
if (tests[i].fn(argc - 1, argv + 1) == 0) {
tests[tone_test_index].fn(2, tone_pass); // Play a notification.
Copy link
Member

Choose a reason for hiding this comment

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

Is the tone guaranteed to be linked?
If not we need a guard on int tone_test_index == -1 So if tone is not found to not invoke the test at 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Thank you! Guards added, initialized to -1.

I made another pass through test_runner and changed loop index types to size_t (index type) and condensed a few printf() calls as well. Those changes are pushed up and rebased with current master. Tested on pixhawk 4 (V5) and performing as expected - "tests time" still fails to return properly as reported in #11361, (not caused by this PR).

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 2 times, most recently from e7b2a80 to fc79dca Compare February 5, 2019 16:58
davids5
davids5 previously approved these changes Feb 5, 2019
View reviewed changes
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@mcsauder LGTM!

@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch 4 times, most recently from df70466 to 0abf07f Compare February 8, 2019 21:42
@weekly-digest weekly-digest bot mentioned this pull request Feb 10, 2019
Closed
@mcsauder mcsauder force-pushed the test_hrt_cpp_refactor branch from 0abf07f to fa6331a Compare February 10, 2019 17:22
@dagar dagar merged commit 2fa70fc into PX4:master Feb 10, 2019
@mcsauder
Copy link
Contributor Author

Thanks @dagar and @davids5!

@mcsauder mcsauder deleted the test_hrt_cpp_refactor branch February 11, 2019 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagar dagar dagar approved these changes

@davids5 davids5 davids5 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcsauder @davids5 @dagar