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

Tunes library #8679

Merged
merged 35 commits into from
Jan 29, 2018
Merged

Tunes library #8679

merged 35 commits into from
Jan 29, 2018

Conversation

LorenzMeier
Copy link
Member

This is a significant cleanup in terms of how we play tunes.

Simone Guscetti and others added 30 commits January 14, 2018 13:14
cmake configs: Modify to include new library
to allign with the libray output which is in us
Now it is possible to set a string and a control.
The get_next_tune function use the same return values as the erliaer
implemented parse_cmd and parse_string functions.
- Start tone_alrm driver after uorb
- Replace tone_alarm $TUNE_ERR with tune_control play -m ${TUNE_ERR}
- TUNE_ERR is a string
@santiago3dr
Copy link

ran flash, fight a couple calibrations, and booting without sd card on pixracer (V4)
https://logs.px4.io/plot_app?log=3b70ab97-d593-4adc-83ef-75df5a6a7228

only difference I could find from master is the startup tones
I have disabled the safety switch so on bootup I normally don't get the startup tone but rather the beep from when you activate the safety switch.
On this PR, with the safety switch disabled, I now get the regular startup tones.

@Avysuarez
Copy link

flight with pixhawk pro (V4pro) only diference is the tone of startup
https://review.px4.io/plot_app?log=1b10dea3-2009-46f2-a517-ac879cd14ddf

flight with pixhawk 1 (V2) only diference is the tone of startup
https://review.px4.io/plot_app?log=69d87bff-398a-4a56-af95-d973baffdee8

@r0gelion
Copy link
Contributor

Is the tune at the startup the expected tune?

@dagar
Copy link
Member

dagar commented Jan 17, 2018

Overall looks quite good, it's just a lot to go through to verify we haven't lost any functionality.

Copy link
Contributor

@r0gelion r0gelion left a comment

Choose a reason for hiding this comment

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

Flight testing went well, we didn't find issues. There is a difference on the startup tune, but this doesn't affect functionality.

@simonegu
Copy link
Contributor

simonegu commented Jan 19, 2018

@r0gelion what is the difference? Is the tune different, or the time it is played?

@santiago3dr
Copy link

quick video to show the tones we expect from master and those of this PR:
https://youtu.be/6_QM0Qf3HP0

@potaito
Copy link
Contributor

potaito commented Jan 23, 2018

can you hold off with merging? I might have a few fixes for this branch. Mostly about corner cases.

@simonegu
Copy link
Contributor

@santiago3dr thanks a lot for the video. So it seems that the tune sequence gets always played. I personally like that but I leave the decision to @LorenzMeier or @dagar.

@simonegu simonegu changed the title Tunes library [WIP] Tunes library Jan 23, 2018
@dagar
Copy link
Member

dagar commented Jan 29, 2018

@potaito what issues?

@dagar
Copy link
Member

dagar commented Jan 29, 2018

Overall the PR looks quite good. We need to do a pass to bring SITL in line, but I don't see any reason we can't merge this now and continue iterating.

@dagar dagar changed the title [WIP] Tunes library Tunes library Jan 29, 2018
@dagar dagar merged commit 47e5382 into master Jan 29, 2018
@dagar
Copy link
Member

dagar commented Jan 29, 2018

Thanks everyone!

@dagar dagar deleted the simonegu-pr_tunes_library branch January 29, 2018 14:46
@potaito
Copy link
Contributor

potaito commented Jan 29, 2018

@dagar you gave me 5 minutes to reply? 🙃 I'm living on a fork and was not sure what issues are caused by the tunes library itself and what are caused by the (possibly wrong) way I am using it. That's why I was not specific. "Corner cases" I mentioned involve this uninitialized guy for example and repeated tunes not being played back correctly:
https://github.com/PX4/Firmware/blob/47e5382e7efa68bc3f34fd767a1a76193610112c/src/lib/tunes/tunes.h#L100

@dagar
Copy link
Member

dagar commented Jan 29, 2018

@potaito I didn't see anything critical and wanted to get this in before it needed yet another rebase (one of the reasons this has dragged on for months). Let's open a new issue or PR to keep working on any issues.

@bkueng bkueng mentioned this pull request Jan 30, 2018
@bkueng
Copy link
Member

bkueng commented Jan 30, 2018

systemcmds/tune_control was only added to the v4 (pixracer) target. I added the rest in #8679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants