-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Utilize DEFINE_PARAMETERS() macro for params in mavlink_receiver.cpp/h #11274
Conversation
|
||
orb_advert_t _actuator_controls_pubs[4] = {nullptr, nullptr, nullptr, nullptr}; | ||
|
||
orb_advert_t _accel_pub = nullptr; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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!
d439184
to
3ddfe25
Compare
3ddfe25
to
59c6341
Compare
|
||
static const int _gps_inject_data_queue_size{6}; | ||
|
||
int _actuator_armed_sub{0}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Taken care of!
7bb92e3
to
27db1a4
Compare
uint8_t _mom_switch_pos[MOM_SWITCH_COUNT] {}; | ||
uint16_t _mom_switch_state{0}; | ||
|
||
param_t _p_bat_crit_thr{0}; |
There was a problem hiding this comment.
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.
param_t _p_bat_crit_thr{0}; | |
param_t _p_bat_crit_thr{PARAM_INVALID}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
d725ca1
to
5660f10
Compare
px4_write(fd, tune, strlen(tune) + 1); | ||
px4_close(fd); | ||
} else { | ||
orb_publish(ORB_ID(tune_control), _tune_control_sub, &tune); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong? https://github.com/PX4/Firmware/blob/master/msg/tune_control.msg
There was a problem hiding this comment.
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:
-
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.
-
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eb5813a
to
09a7d94
Compare
5f1e25d
to
976ec18
Compare
976ec18
to
87893db
Compare
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")}; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fixed up.
d761ba8
to
fad186d
Compare
5c4be13
to
a8cf3e5
Compare
1d03949
to
a5d37c6
Compare
Flight tested here: Spot test requesting the battery params during flight can be seen in this screenshot: |
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! |
int _orb_class_instance{-1}; | ||
int _param_sub{-1}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bkueng ! Fixed up!
a5d37c6
to
cecb19d
Compare
@@ -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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davids5 and @dagar , I bench tested the latest commit in this PR on a pixhawk4 mini and pixhawk 4 by adding a 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! |
Thanks @bkueng, @julianoes , @dagar , and @davids5 for each of your reviews on this PR! |
@@ -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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…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.
…lace in mavlink_receiver.h.
7f87640
to
a0a5279
Compare
Rebased with current master. Please let me know if you would like the commits squashed. Thanks for all of your review efforts! |
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. |
Here is a screenshot of basic bench test exercising the affected methods in |
Thanks @bkueng ! |
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 thanpx4_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