Skip to content

Conversation

@bjv-capra
Copy link
Collaborator

This PR resolves #16

@bjv-capra bjv-capra requested a review from norro November 5, 2021 09:28
@bjv-capra
Copy link
Collaborator Author

I didn't want to remove the updater_id from the updater struct. But at the same time, there's nothing setting it up. So I didn't know what to do with it ATM.

@bjv-capra
Copy link
Collaborator Author

@norro friendly ping. Thoughts?

@norro
Copy link
Contributor

norro commented Nov 12, 2021

Sorry @bjv-capra, I am pretty swampedright now. I will have a look at it in the afternoon or beginning of next week.

Copy link
Contributor

@norro norro left a comment

Choose a reason for hiding this comment

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

It took me a bit, because I wanted to understand that this PR is still allowing to emulate the current setup. For this I created ran the entire pipeline from example updaters to bridge to aggregator to robot monitor (sort of what I suggest to do automatically in #28).

All in all the PR looks good to me. It seems that just the example updaters (MICRO_ROS_DIAGNOSTIC_UPDATER_EXAMPLES=True) and a bit of documentation is necessary.

Thank you for the suggestion and implementation. :)

@bjv-capra
Copy link
Collaborator Author

Regarding the examples, why would we enable them by default? I'll see to update the documentation.

@norro
Copy link
Contributor

norro commented Nov 18, 2021

Oh, sorry, I didn't mean to make them default, just to update them. Currently, they don't build properly in this PR, if someone would enable them.

@bjv-capra
Copy link
Collaborator Author

Got it, I'll update them and update the PR.

@bjv-capra
Copy link
Collaborator Author

bjv-capra commented Nov 22, 2021

Just FYI, while updating the PR I noticed that the system default QoS drops messages. I couldn't quite find the default values of the struct. I changed it to be reliable and keep_last(10).

I also refactored the examples a bit, I still need to update the Readme

EDIT: volatile had nothing to do with the issue.

@norro
Copy link
Contributor

norro commented Nov 22, 2021

Agreed, the default QoS does not seem to be appropriate.
Off-topic: I wonder if we would want to suggest a diagnostics default QoS. Currently there is just default, service, sensor, parameter, see .

@bjv-capra
Copy link
Collaborator Author

@norro Any clue why the pipeline fails to build? It says it doesn't know rclc_publisher_init_default which is odd...

Off-topic, wouldn't be a bad idea, as diagnostics is quite a widely used feature.

@bjv-capra bjv-capra marked this pull request as ready for review November 22, 2021 09:55
@norro
Copy link
Contributor

norro commented Nov 23, 2021

I can reproduce this error in my local setup, but don't really understand this either. The symbol rclc_publisher_init_default is contained in the rclc library, that I can see.

@bjv-capra
Copy link
Collaborator Author

Seems I was running with an old version of galactic. I updated it and I do get the same error now. But I don't quite understand why. Do I need to build rclc from source?

@bjv-capra
Copy link
Collaborator Author

I think I found a possible fix

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #25 (f64e497) into master (fd90365) will decrease coverage by 0.17%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   39.20%   39.02%   -0.18%     
==========================================
  Files           2        2              
  Lines         125      123       -2     
  Branches       19       19              
==========================================
- Hits           49       48       -1     
+ Misses         62       61       -1     
  Partials       14       14              
Impacted Files Coverage Δ
micro_ros_common_diagnostics/src/hwmonitor.c 0.00% <0.00%> (ø)
..._diagnostic_updater/micro_ros_diagnostic_updater.c 71.64% <100.00%> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd90365...f64e497. Read the comment docs.

@bjv-capra
Copy link
Collaborator Author

@norro this is ready now

Copy link
Contributor

@norro norro left a comment

Choose a reason for hiding this comment

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

Thank for the PR, @bjv-capra. I think diagnostics get more suitable for micro controllers now with these changes.

@bjv-capra bjv-capra merged commit 0ab4182 into micro-ROS:master Nov 25, 2021
@norro
Copy link
Contributor

norro commented Nov 25, 2021

@bjv-capra Just seconds before your merge, I realized that you forgot to sign your commits, which is part of our contribution guideline (see https://github.com/micro-ROS/micro_ros_diagnostics/blob/master/CONTRIBUTING.md#sign-your-work).
Can you sign your latest commits and force-push them to master, please?

I added the DCO check to the PR checks now for the future.

@bjv-capra
Copy link
Collaborator Author

I tried to force-push and got

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "build (ubuntu-20.04, rolling)" is expected. At least 1 approving review is required by reviewers with write access.
To github.com:micro-ROS/micro_ros_diagnostics.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:micro-ROS/micro_ros_diagnostics.git'

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.

Support multiple HW id per updater

3 participants