Add mavlink play tune message to simulator class #11431

catch-twenty-two · 2019-02-11T01:43:28Z

This revisits and finishes up the ability to send a tone alarm over mavlink to a simulator when in simulator mode. There is a JMAVSim update that adds a 'peripheral class' which takes advantage of the new message here:

PX4/jMAVSim#92

Original concept/prototype demonstration is here (this pr adds same functionality):

https://www.youtube.com/watch?v=t7cvCfbbXbE

dagar · 2019-02-11T01:49:08Z

@mcsauder

mcsauder · 2019-02-12T15:56:38Z

src/modules/simulator/simulator_mavlink.cpp

+			struct {
+				unsigned int frequency;
+				unsigned int duration;
+				unsigned int silence_length;


@catch-twenty-two , if you change silence_length to silence you will match the naming convention of the tune_control_s type. (You could consider utilizing that type to store these values, although it could require modifying how you are using the union's memory space to transfer values to the message.)

I ended up removing the union, I didn't really need it any more. I still create a local structure just to be on the safe side in case the tune_control_s structure changes, and I think it makes the code a little more readable.

mcsauder · 2019-02-12T15:58:03Z

@catch-twenty-two and @dagar , this PR looks good to me.

@dagar, do we need to add orb_unsubscribe calls to this class's destructor (and should the constructor/destructor get moved out of the header file)?

EDIT: I see now that the class is broken across a few files, so the constructor/destructor definitions need to stay put.

mcsauder · 2019-02-12T15:59:55Z

src/modules/simulator/simulator_mavlink.cpp

+				unsigned int duration;
+				unsigned int silence_length;
+			};
+			uint8_t a[sizeof(tune_msg.tune)];


@catch-twenty-two , I'm not a huge fan of single letter variables outside of index usage,... Any chance we could come up with a more descriptive name for a?

this is gone now

mcsauder · 2019-02-15T01:55:25Z

@catch-twenty-two , can you resolve the merge conflicts and push up a rebase ?

https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Here's the gist:

git fetch upstream
git merge upstream/master
git merge-base upstream/master YOUR_BRANCH_NAME    (which happens to be master)

some commit sha will magically appear, copy it to paste it later

git rebase -i THE_REBASE_COMMIT_SHA_YOU_JUST_COPIED

ctrl+O to write Out, ctrl+X to save and eXit

review your commit comments and update as needed...
ctrl +O to write Out, ctrl+X to save and eXit

git push -f

force push your rebased changes to the PR. Done!

find and message me on PX4 slack if you need a hand.

mcsauder · 2019-02-15T01:58:47Z

@dagar and @catch-twenty-two , I have work ready to follow this PR as a clean-up pass.

…ulator mode

catch-twenty-two · 2019-02-16T04:43:56Z

@mcsauder Something went wrong with my rebase, please hold off on merging this until I fix it...I seem to have a copy and paste issue

catch-twenty-two · 2019-02-16T04:48:01Z

Okay should be all good now

mcsauder · 2019-02-16T05:03:56Z

@catch-twenty-two , in case you find it helpful, if you'd like to have a tidy commit history that shows the steps you'd like to have followed in future code forensics, you can use a handy rebase command, git rebase -i HEAD~2 to allow you to squash the last two commits into one, or HEAD~3 to squash the last 3 into one, etc.. This also allows you to edit the commit comments to illustrate your intent.

NOTE: It is always tricky re-writing history in a repo, and there can be consequences, so be aware of them, but the very best time to do it is before it becomes part of the permanent record in in the main repository. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

Good work so far!

mcsauder · 2019-02-16T05:56:37Z

See also PR #11447

mcsauder · 2019-02-16T05:57:45Z

@dagar and @davids5 , looks good to me.

bkueng · 2019-02-18T10:24:31Z

src/modules/simulator/simulator_mavlink.cpp

+			unsigned int silence;
+		} tune_info = {};
+
+		_tunes.get_next_tune(tune_info.frequency, tune_info.duration, tune_info.silence);


It's not enough to call this only when the topic gets updated. I think we need similar logic as in ToneAlarm.cpp.

@bkueng got it, ill take a look at the logic and try to replicate it here.

It should be fixed now, Thanks for the help.

catch-twenty-two · 2019-02-19T06:36:23Z

I've found a solution, working on it now...

potaito · 2019-02-21T14:40:06Z

src/modules/simulator/simulator_mavlink.cpp

+			unsigned int silence;
+		} tone_info = {};
+
+		if (_tunes.get_next_note(tone_info.frequency, tone_info.duration, tone_info.silence) > 0) {


I'm not saying you must, but you could additionally check if the note you are going to send is audible with the condition ... && tone_info.frequency > 0 && tone_info.duration > 0 && tone_info.strength > 0. Then you won't send the pause notes.

LorenzMeier · 2019-03-03T10:07:27Z

@catch-twenty-two Please rebase to fix CI

catch-twenty-two · 2019-03-04T06:29:23Z

@catch-twenty-two Please rebase to fix CI

Done, thx

catch-twenty-two · 2019-03-06T04:07:17Z

squashed commit

bkueng

Looks almost good, thanks for the update.
One more thing: can we disable it by default, e.g. via CBRK_BUZZER parameter? If you're sitting in a room with people all running the sim, it's not cool to hear the tunes all the time ;)

bkueng · 2019-03-06T07:01:48Z

src/modules/simulator/simulator_mavlink.cpp

@@ -782,6 +825,8 @@ void Simulator::poll_for_MAVLink_messages()
 	_actuator_outputs_sub = orb_subscribe_multi(ORB_ID(actuator_outputs), 0);
 	_vehicle_status_sub = orb_subscribe(ORB_ID(vehicle_status));

+	_tune_control_sub = orb_subscribe(ORB_ID(tune_control));


This and the other topics are missing an orb_unsubscribe at the bottom of the method.

Hi @bkueng , I think I can see why the CI tests here are unhappy... do you have a preference about where the unsubscribe methods get placed?

In that case at the end of poll_for_MAVLink_messages.

Apologies, I didn't make things very clear by pointing to those commits,

This and the other topics are missing an orb_unsubscribe at the bottom of the method

In that case at the end of poll_for_MAVLink_messages.

Indeed, they are the same.

Adding the orb unsubscribe calls at the end of this method causes a segfualt. Placing the unsubscribe() calls in the destructor does not. Additional usage of the subscription vars occurs in send(), send_controls, and poll_topics(). The destructor seems to be the appropriate location for the unsubscribe() calls given their usage outside of poll_for_MAVLINK_messages(), but I can find no other examples where subscriptions take place outside of the constructor, (in a class method), and then unsubscribed in the destructor.

It seems appropriate to move these subscribe calls to the constructor in this case, but let me know if you have other feedback in PR #11631. Thanks!

Adding the orb unsubscribe calls at the end of this method causes a segfualt. Placing the unsubscribe() calls in the destructor does not.

If I saw it correctly, you called unsubscribe still within the loop, not at the very end of poll_for_MAVLINK_messages.

The destructor seems to be the appropriate location for the unsubscribe()

If you subscribe in the constructor it's the right place. That's my prefered solution as well, and it should work with the current structure (refactoring to ModuleBase is another thing we can do).

bkueng · 2019-03-06T07:04:38Z

src/modules/simulator/simulator_mavlink.cpp

+
+void Simulator::send_sim_tone()
+{
+	static long tone_timer = 0;


Can you make this a class attribute?

…y-tune

catch-twenty-two · 2019-07-19T01:05:31Z

All pr review requests should be addressed now

bkueng

There are still problems, can you check please?

bkueng · 2019-07-19T08:02:11Z

src/modules/simulator/simulator_mavlink.cpp

@@ -759,6 +792,8 @@ void Simulator::poll_for_MAVLink_messages()
 	_actuator_outputs_sub = orb_subscribe_multi(ORB_ID(actuator_outputs), 0);
 	_vehicle_status_sub = orb_subscribe(ORB_ID(vehicle_status));

+	_tune_control_sub = orb_subscribe(ORB_ID(tune_control));


I don't see where this is unsubscribed.

bkueng · 2019-07-19T08:04:56Z

src/modules/simulator/simulator_mavlink.cpp

+			unsigned int silence;
+		} tune_info = {};
+
+		_tunes.get_next_note(tune_info.frequency, tune_info.duration, tune_info.silence);


I think I raised this before, you need to move this out of the updated condition. A single tune_control topic update might lead to several notes.

bkueng · 2019-07-19T08:05:11Z

src/modules/simulator/simulator_mavlink.cpp

+
+	if (updated) {
+
+		orb_copy(ORB_ID(_control), _control_sub, &_tune_control);


Suggested change

orb_copy(ORB_ID(_control), _control_sub, &_tune_control);

orb_copy(ORB_ID(tune_control), _control_sub, &_tune_control);

bkueng · 2019-07-19T08:06:41Z

src/modules/simulator/simulator_mavlink.cpp

+			_cbrk = circuit_breaker_enabled("CBRK_BUZZER", CBRK_BUZZER_KEY);
+		}
+
+		if (_cbrk == CBRK_OFF) {


Please make sure this is set by default for SITL

bkueng · 2019-07-19T08:07:10Z

src/modules/simulator/simulator.h

@@ -293,10 +297,15 @@ class Simulator : public ModuleParams
 	uint64_t _hil_ref_timestamp{0};

 	// uORB data containers
+


Unnecessary newline

bkueng · 2019-07-19T08:07:45Z

src/modules/simulator/simulator.h

+	tune_control_s _tune_control {};
+
+	Tunes _tunes;
+	int _cbrk{CBRK_UNINIT};     		///< If true, no audio output.


It's an int, so true is not appropriate here

catch-twenty-two · 2019-09-12T13:49:32Z

Hi @mcsauder, I'm going to close this pr until I have time to finish it up. I just don't have the bandwidth right now.

mcsauder · 2019-09-17T03:16:23Z

I think this is good work, so I'll try to carry it on in #12507. (Please re-open if you'd like it to stick around!)

…ded #includes, add volume to send_sim_tone() message and minor formatting.

catch-twenty-two mentioned this pull request Feb 11, 2019

adding new peripheral class as a container for simulator and activating simulated buzzer code PX4/jMAVSim#92

Closed

mcsauder reviewed Feb 12, 2019

View reviewed changes

catch-twenty-two added 3 commits February 15, 2019 20:08

adding ability for tune control to send mavlink play tune info in sim…

adbebae

…ulator mode

fixing whitespace

98646cd

simplifying code

2227389

catch-twenty-two force-pushed the master branch from df5ce64 to e695ba5 Compare February 16, 2019 04:17

bkueng reviewed Feb 18, 2019

View reviewed changes

catch-twenty-two force-pushed the master branch 2 times, most recently from e42ede3 to 271937b Compare February 20, 2019 17:36

potaito reviewed Feb 21, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Feb 24, 2019

Weekly Digest (17 February, 2019 - 24 February, 2019) #11532

Closed

catch-twenty-two force-pushed the master branch from 9771ddc to 465f769 Compare March 4, 2019 06:20

catch-twenty-two force-pushed the master branch 2 times, most recently from df4652e to dbec7b4 Compare March 6, 2019 04:06

bkueng reviewed Mar 6, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Mar 10, 2019

Weekly Digest (3 March, 2019 - 10 March, 2019) #11614

Closed

julianoes added Admin: Enhancement (improvement) 💡 Sim: SITL software in the loop simulation labels Jul 18, 2019

Merge branch 'master' of github.com:PX4/Firmware into pr-sim-send-pla…

ddfb568

…y-tune

catch-twenty-two force-pushed the master branch from dbec7b4 to 7fcbc5c Compare July 19, 2019 00:45

catch-twenty-two force-pushed the master branch from 7fcbc5c to dfb752c Compare July 19, 2019 01:03

address pr review requests

352e9dd

catch-twenty-two force-pushed the master branch from dfb752c to 352e9dd Compare July 19, 2019 01:31

bkueng requested changes Jul 19, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #12528

Closed

weekly-digest bot mentioned this pull request Sep 15, 2019

Weekly Digest (8 September, 2019 - 15 September, 2019) #12959

Closed

mcsauder closed this Sep 17, 2019

mcsauder added a commit to mcsauder/PX4-Autopilot that referenced this pull request Oct 18, 2019

Accomplish the requisite work to close out PR PX4#11431: remove unnee…

369645d

…ded #includes, add volume to send_sim_tone() message and minor formatting.


		if (updated) {

		orb_copy(ORB_ID(_control), _control_sub, &_tune_control);

	orb_copy(ORB_ID(_control), _control_sub, &_tune_control);
	orb_copy(ORB_ID(tune_control), _control_sub, &_tune_control);

		@@ -293,10 +297,15 @@ class Simulator : public ModuleParams
		uint64_t _hil_ref_timestamp{0};

		// uORB data containers

Add mavlink play tune message to simulator class #11431

Add mavlink play tune message to simulator class #11431

Conversation

catch-twenty-two commented Feb 11, 2019 • edited by AuterionWrikeBot Loading

dagar commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder commented Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder commented Feb 15, 2019 • edited Loading

mcsauder commented Feb 15, 2019

catch-twenty-two commented Feb 16, 2019 • edited Loading

catch-twenty-two commented Feb 16, 2019

mcsauder commented Feb 16, 2019

mcsauder commented Feb 16, 2019

mcsauder commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catch-twenty-two commented Feb 19, 2019

potaito Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

LorenzMeier commented Mar 3, 2019

catch-twenty-two commented Mar 4, 2019

catch-twenty-two commented Mar 6, 2019

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catch-twenty-two commented Jul 19, 2019

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catch-twenty-two commented Sep 12, 2019

mcsauder commented Sep 17, 2019

catch-twenty-two commented Feb 11, 2019 •

edited by AuterionWrikeBot

Loading

mcsauder commented Feb 12, 2019 •

edited

Loading

mcsauder commented Feb 15, 2019 •

edited

Loading

catch-twenty-two commented Feb 16, 2019 •

edited

Loading

potaito Feb 21, 2019 •

edited

Loading