Fix #547 Add callback for components discovered. #556
Conversation
Use call_user_callback() instead of calling the users function directly.
core/include/system.h
Outdated
| /** | ||
| * @brief Register a callback to be called when a component is discovered. | ||
| * | ||
| * @param callback a function of type void(uint8_t) which will be called with the COMPONENT_ID |
There was a problem hiding this comment.
I would prefer if we don't "leak" the mavlink component ID outside of the SDK but only the notion of autopilot/camera/gimbal. Therefore, in my opinion, it should be something like register_camera_discovered().
There was a problem hiding this comment.
Is there a convention on how components should be called? I mean how do you know, from MAVLink, that it is a camera?
There was a problem hiding this comment.
The camera has MAV_COMP_ID_CAMERA[x], it's something known to the SDK internals.
https://mavlink.io/en/messages/common.html#MAV_COMP_ID_CAMERA
There was a problem hiding this comment.
@julianoes Can we declare an enum like ::Camera ::Gimbal ::AutoPilot ::Unknown etc (maybe this already exists and pass that to the callback? that way there'd only be one callback function instead of one function per component.
There was a problem hiding this comment.
Yes an enum for components makes sense, I agree.
There was a problem hiding this comment.
@julianoes change pushed.
There was a problem hiding this comment.
Cool, the comment needs to be adapted though.
…aning callback can be registered at any time.
…ent discovered callback so we dont leak the Mavlink IDs.
| /** | ||
| * @brief type for component discovery callback | ||
| */ | ||
| typedef std::function<void(ComponentType)> discover_callback_t; |
core/include/system.h
Outdated
| /** | ||
| * @brief Register a callback to be called when a component is discovered. | ||
| * | ||
| * @param callback a function of type void(uint8_t) which will be called with the COMPONENT_ID |
There was a problem hiding this comment.
Cool, the comment needs to be adapted though.
core/system_impl.cpp
Outdated
| case MAV_COMP_ID_CAMERA: | ||
| case MAV_COMP_ID_CAMERA2: | ||
| case MAV_COMP_ID_CAMERA3: | ||
| case MAV_COMP_ID_CAMERA4: |
There was a problem hiding this comment.
Next time we'll have to refactor this switch case into a function.
There was a problem hiding this comment.
Oh, just seing it after my comment above. So that is duplicated from above, and should become something like const auto component_type = getComponentType(component_id);.
There was a problem hiding this comment.
Looking great! On my side, the duplicated code should be removed, and the comments fixed, and then I'll be fine with it!
Thank you @dbowerman :-)
core/system_impl.cpp
Outdated
| @@ -332,6 +334,29 @@ void SystemImpl::add_new_component(uint8_t component_id) | |||
| { | |||
| auto res_pair = _components.insert(component_id); | |||
| if (res_pair.second) { | |||
| if (component_discovered_callback != nullptr) { | |||
| ComponentType component_type; | |||
There was a problem hiding this comment.
Small readability thing: I would put that in a function, e.g. getComponentType(const uint8_t component_id) (and here do const auto component_type = getComponentType(component_id). What do you think?
core/system_impl.cpp
Outdated
| case MAV_COMP_ID_CAMERA: | ||
| case MAV_COMP_ID_CAMERA2: | ||
| case MAV_COMP_ID_CAMERA3: | ||
| case MAV_COMP_ID_CAMERA4: |
There was a problem hiding this comment.
Oh, just seing it after my comment above. So that is duplicated from above, and should become something like const auto component_type = getComponentType(component_id);.
|
@julianoes @JonasVautherin comments addressed :) |
| default: | ||
| return UNKNOWN; | ||
| } | ||
| } |
|
Testing right now, will merge ASAP. |
Add the ability to register a callback so the application can be notified when individual components are added.
See example/takeoff_land for usage example.