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

Utilize DEFINE_PARAMETERS() macro for params in mavlink_receiver.cpp/h #11274

Merged
merged 3 commits into from
Jun 17, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Jan 22, 2019

Hi,

This PR moves variable initialization out of the MavlinkReceiver constructor into mavlink_reciever.h at the variable declarations and changes the handle_message_play_tune method to utilize uORB rather than px4_open(). It also alphabetizes subscription and variables lists in mavlink_receiver.h.

@davids5, this work partly addresses feedback in PR #11192.

Hardware testing coonducted with QGroundControl 3.4.4 and pixhawk 4 (fmu-v5).

Please let me know if you have any questions on this PR. Thanks!

-Mark


orb_advert_t _actuator_controls_pubs[4] = {nullptr, nullptr, nullptr, nullptr};

orb_advert_t _accel_pub = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky, but what about doing this with uniform initialization? It looks a bit weird at first, but there are advantages.
I've been trying to do is consistently across the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Uniform initialization in c++11 means {}.

eg orb_advert_t _accel_pub{nullptr};

https://mbevin.wordpress.com/2012/11/16/uniform-initialization/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dagar ! Done and pushed up!


static const int _gps_inject_data_queue_size{6};

int _actuator_armed_sub{0};
Copy link
Member

Choose a reason for hiding this comment

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

These file descriptors (orb subscriptions) should be initialized to -1 (something invalid). Zero can be valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Taken care of!

@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 2 times, most recently from 7bb92e3 to 27db1a4 Compare January 23, 2019 03:54
uint8_t _mom_switch_pos[MOM_SWITCH_COUNT] {};
uint16_t _mom_switch_state{0};

param_t _p_bat_crit_thr{0};
Copy link
Member

@dagar dagar Jan 23, 2019

Choose a reason for hiding this comment

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

Similar to the subscription, 0 is actually a valid param id.

Suggested change
param_t _p_bat_crit_thr{0};
param_t _p_bat_crit_thr{PARAM_INVALID};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 2 times, most recently from d725ca1 to 5660f10 Compare January 23, 2019 04:05
px4_write(fd, tune, strlen(tune) + 1);
px4_close(fd);
} else {
orb_publish(ORB_ID(tune_control), _tune_control_sub, &tune);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@davids5 davids5 Jan 23, 2019

Choose a reason for hiding this comment

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

@dagar, @mcsauder - It was broken since tune control came in. Passing a string to a non serviced IOCTL. The first decision is: Do we need it? If not remove it and return and error.
If we need it there are 2 choices:

  1. Simple: Add static member in the tune classes that owns the compiled tune desc file to look up the ascii string in the tone table and return its ordinal. If it returns -1 issues an error and exit.

  2. Complex: Add code with resource protection in the tune lib to play an arbitrary tune string like as done in the tune_control app but make it non blocking. This restores the functionally to how it was but not the Real Time nature that it had off the HRT .

My preference is #1 with a fall back that if in the future someone says it is broken, we can always do #2 but that is based on the features value and my work load. On the other hand if someone has time wants to learn, this is a good little project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagar, I reverted this section to help this PR move forward. @davids5 , I'll get the IOCTL work finished up in a follow-up PR.

@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 3 times, most recently from eb5813a to 09a7d94 Compare January 24, 2019 01:04
@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 11 times, most recently from 5f1e25d to 976ec18 Compare February 4, 2019 04:03
@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch from 976ec18 to 87893db Compare February 4, 2019 22:59
uint8_t _mom_switch_pos[MOM_SWITCH_COUNT] {};
uint16_t _mom_switch_state{0};

param_t _p_bat_crit_thr{param_find("BAT_CRIT_THR")};
Copy link
Member

Choose a reason for hiding this comment

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

Since you're already changing this I'd suggest looking at the newer px4_params.h c++ template. It will actually catch parameter problems at compile time and you don't need to keep the param_t.
EKF2 example - https://github.com/PX4/Firmware/blob/master/src/modules/ekf2/ekf2_main.cpp#L292

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All fixed up.

@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 2 times, most recently from d761ba8 to fad186d Compare February 6, 2019 16:07
@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch from 5c4be13 to a8cf3e5 Compare June 1, 2019 17:02
@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch 3 times, most recently from 1d03949 to a5d37c6 Compare June 7, 2019 18:50
@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 8, 2019

Flight tested here:
https://review.px4.io/plot_app?log=1d103e72-aa9d-448a-898e-d043949ec6ab

Spot test requesting the battery params during flight can be seen in this screenshot:
image

@mcsauder
Copy link
Contributor Author

Hi @bkueng , would you mind giving this PR your trained eye for a few minutes? I am hoping to get it merged or closed soon.

I appreciate any feedback you have. Thank you!

julianoes
julianoes previously approved these changes Jun 12, 2019
int _orb_class_instance{-1};
int _param_sub{-1};
Copy link
Member

Choose a reason for hiding this comment

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

This never gets initialized. You can use uORB::Subscription _param_update_sub{ORB_ID(parameter_update)}; now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @bkueng ! Fixed up!

@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch from a5d37c6 to cecb19d Compare June 13, 2019 16:23
@@ -2589,6 +2566,11 @@ MavlinkReceiver::receive_thread(void *arg)
hrt_abstime last_send_update = 0;

while (!_mavlink->_task_should_exit) {
// Check for updated parameters.
if (_param_update_sub.updated()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagar and @davids5 , this check mirrors the Navigator::run() method while loop check for updated parameters. This should prevent the issue discussed during the dev call checking for updated params every iteration of the receive_thread() while loop.

@mcsauder
Copy link
Contributor Author

@davids5 and @dagar , I bench tested the latest commit in this PR on a pixhawk4 mini and pixhawk 4 by adding a PX4_INFO(update_params()) printout in the body of the update_params() method then setting parameters for each of the affected handle_message_XXX() methods:
image

Flight log from this PR here: https://review.px4.io/plot_app?log=d85db144-2341-4c60-945d-1ff167826cf5

Flight log from master here: https://review.px4.io/plot_app?log=2f3ecfb0-c665-4971-bd21-76b72f454120

This PR is good to go. Please let me know if you have any additional questions or concerns on this PR!

@mcsauder
Copy link
Contributor Author

Thanks @bkueng, @julianoes , @dagar , and @davids5 for each of your reviews on this PR!

@mcsauder
Copy link
Contributor Author

@dagar or @davids5 , if not finalized by next week, could you please add this to the dev call? Thank you!

@@ -188,11 +187,17 @@ class MavlinkReceiver

void send_storage_information(int storage_id);

/**
* @brief Checks for updated uORB parameters and updates the params if required.
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect as it does not check. Can you either move the check in here or update the comment?
Other than that this looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I pushed up a correction.

mcsauder added 3 commits June 14, 2019 08:33
…inkReceiver class, alphabetize var lists. Added update_params() method to ensure that parameters subscribed to are updated routinely.
…re-implement the update_params() method and only call if parameters have been updated.
@mcsauder mcsauder force-pushed the mavlink_reciever_var_initialization branch from 7f87640 to a0a5279 Compare June 14, 2019 14:33
@mcsauder
Copy link
Contributor Author

Rebased with current master. Please let me know if you would like the commits squashed. Thanks for all of your review efforts!

@dagar
Copy link
Member

dagar commented Jun 14, 2019

Looks right, I'd just do something quick to test that it has the parameter values initially (eg UUID in mavlink flight_information) and that it's picking up changes.

@mcsauder
Copy link
Contributor Author

Here is a screenshot of basic bench test exercising the affected methods in MavlinnkReceiver on pixhawk 4. Test performed by first querying the param, then setting the param, then querying the param to verify the set value with the following: COM_FLIGHT_UUID, BAT_CRIT_THR, BAT_EMERGEN_THR, SENS_FLOW_MAXHGT, and SENS_FLOW_ROT.

image

@bkueng bkueng merged commit 8811c83 into PX4:master Jun 17, 2019
@mcsauder
Copy link
Contributor Author

Thanks @bkueng !

@mcsauder mcsauder deleted the mavlink_reciever_var_initialization branch June 17, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants