-
Notifications
You must be signed in to change notification settings - Fork 20
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
Issue 203 cycle switch mode #212
Issue 203 cycle switch mode #212
Conversation
Requires a branch,"motoros2_issue_203_cycle_switch_mode", in motoros2_interfaces
GetNotReadySubCode and StartMotionMode now have same checking order
Depends on Yaskawa-Global/motoros2_interfaces#10 Code looks good to me, and I saw yai-rosejo test it. @gavanderhoorn, do you have any concerns about message version compatiblity? |
no, not in this case. We're not embedding the |
High-level comment: should we perhaps also post a (non-blocking) alarm? That would make it really obvious something is misconfigured (at least for use with MotoROS2). |
I don't think so. I don't think it's any different than the other motion-not-ready codes. The status message indicates What would be your argument for an alarm? |
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.
I think this is in good shape. Merging.
I'm almost certain this PR broke CI builds. The msbuild filter did determine the PR needed a full build, but that didn't happen because @yai-rosejo submitted from a fork. I'm not sure why the merge by @ted-miller didn't trigger a full build either. In any case: CI is probably broken until Lines 72 to 73 in 7574bd2
I'll trigger a manual full build (of MotoROS2) and verify. |
See https://github.com/Yaskawa-Global/motoros2/actions/runs/8048602951. It indeed fails due to the undeclared constants. Note: @yai-rosejo: this comment is not to put blame on anyone, just to draw attention to the fact our CI is currently broken (in two ways). |
@ted-miller wrote:
mostly just to draw extra attention to it. The cycle mode is a rather invisible setting I believe, and perhaps also not too obvious where to change. This in contrast to most (all?) other failure reasons we check for when starting a traj mode. With an alarm, we could also more easily document a solution for users, as we'd have a specific number to refer to. But we don't really need to do anything here. It was just a suggestion. |
I did notice that the CI didn't automatically run, as I expected. Normally, I've seen it just say either success or failure. In this case, I had to manually instantiate it. I'm not sure if this is relevant.
This is something that I didn't consider. How do we handle a PR that require changes to the libmicroros? I guess we would need to make release of that before handling the PR. John and I have been working from source, and not considering this scenario for external users. |
It should also tell you why it failed, but you'd have to go to the specific run and inspect the results. If/when we get an improved
what do you mean by "had to manually instantiate it"?
Current This ties into the discussion around how we version, build and distribute M+ The sort of breakage we discovered here being one of the reasons we're looking into this. |
CI didn't run automatically. There was a button to make it run. I clicked it.
Do you think it's worth reverting this PR in the short-term? (I'm not personally aware of any external users building from source) |
ah, that's probably because the PR was submitted by a new/external contributor. We've configured the repository such that all Action workflows must then be approved before they run. It's not the cause of what we're discussing here.
Technically we only 'release' M+ For now, users building from source can't build |
Fixes #203
Added in a CIO read to check for continuous cycle mode being set