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

I exposed an Option<QosProfile> to the user of the crate. #95

Conversation

TijlJappens
Copy link
Contributor

I exposed an Option to the user of the crate so that he/she can alter the QoS profile for the created services.

…he can alter the QoS profile for the created services.
@m-dahl
Copy link
Collaborator

m-dahl commented May 15, 2024

Hi,

Thanks! I am sure this will be useful to have. A couple of comments:

  • I think we should skip the Option and make the user pass in QosProfile::default() instead of None, then the API matches what's already there for publishers and subscribers.
  • If we expose qos settings for servers we should also add it to clients to match.
  • Need to update the examples as they no longer compile.

…n actual QosProfile. I also did the same for the clients and modified some of the examples.
@TijlJappens
Copy link
Contributor Author

TijlJappens commented May 15, 2024

Thank you for your reply. I made the changes you requested but I am unable to test them as I don't know how to run the pipeline/workflow myself. I tried to run them locally with act without much succes (I couldn't get the master to work running act on windows subsystem 2 for linux, only the ROS CI/docs_no_ros worked).

edid: the reason why act failed for me is due to a panic that I have also seen before myself:

| thread 'msg_types::tests::test_capped_sequence' panicked at library/core/src/panicking.rs:156:5:
| unsafe precondition(s) violated: ptr::copy_nonoverlapping requires that both pointer arguments are aligned and non-null and the specified memory ranges do not overlap
| note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
| thread caused non-unwinding panic. aborting.

I don't know if you have ever seen it emerging? This is from the master branch, so has nothing to do with my changes.

@m-dahl
Copy link
Collaborator

m-dahl commented May 17, 2024

Hi. I think how it works is that the CI will run automatically for your PR once you have a commit in the tree. For now I have to approve each run. PR looks good so we can merge it.

The error you got is interesting and I have never seen it before. But as you can see it happened on the github CI now aswell. The last commit was fine but that was made just before rust 1.78 was released. I just updated to 1.78 on my machine and I can reproduce it locally. So something has changed between rust 1.77 and 1.78 which causes the panic. I made a new issue for it here #96 but I don't have time to look into it right now.

@m-dahl m-dahl merged commit a02a78f into sequenceplanner:master May 17, 2024
1 of 6 checks passed
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.

2 participants