search

Yaskawa-Global / motoros2

ROS 2 (rcl, rclc & micro-ROS) node for MotoPlus-compatible Yaskawa Motoman robot controllers
89 stars 14 forks source link

Issue 203 cycle switch mode #212

Closed yai-rosejo closed 4 months ago

yai-rosejo commented 4 months ago

Fixes #203 Added in a CIO read to check for continuous cycle mode being set

ted-miller commented 4 months ago

Depends on https://github.com/Yaskawa-Global/motoros2_interfaces/pull/10

Code looks good to me, and I saw yai-rosejo test it.

@gavanderhoorn, do you have any concerns about message version compatiblity?

gavanderhoorn commented 4 months ago

@gavanderhoorn, do you have any concerns about message version compatiblity?

no, not in this case.

We're not embedding the MotionReadyEnum msg itself, we just use it to store "constants".

gavanderhoorn commented 4 months ago

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).

ted-miller commented 4 months ago

should we perhaps also post a (non-blocking) alarm

I don't think so. I don't think it's any different than the other motion-not-ready codes.

The status message indicates not-ready and the user will get an appropriate error message if they attempt either of the start services.

What would be your argument for an alarm?

gavanderhoorn commented 4 months ago

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 libmicroros sees a new build and release, because motoros2_interfaces__msg__MotionReadyEnum__NOT_READY_NOT_CONT_CYCLE_MODE_STR and motoros2_interfaces__msg__MotionReadyEnum__NOT_READY_NOT_CONT_CYCLE_MODE are not defined in the current release(s) but are used here:

https://github.com/Yaskawa-Global/motoros2/blob/7574bd2d849efda1ad631c07bdc059b585db5c81/src/ErrorHandling.c#L72-L73

I'll trigger a manual full build (of MotoROS2) and verify.

gavanderhoorn commented 4 months ago

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).

gavanderhoorn commented 4 months ago

@ted-miller wrote:

@gavanderhoorn wrote:

should we perhaps also post a (non-blocking) alarm

I don't think so. I don't think it's any different than the other motion-not-ready codes.

The status message indicates not-ready and the user will get an appropriate error message if they attempt either of the start services.

What would be your argument for an alarm?

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.

ted-miller commented 4 months ago

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.

motoros2_interfacesmsgMotionReadyEnum__NOT_READY_NOT_CONT_CYCLE_MODE are not defined in the current release(s) but are used

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.

gavanderhoorn commented 4 months ago

I did notice that the CI didn't automatically run, as I expected. Normally, I've seen it just say either success or failure.

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 mpBuilder, GitHub should show annotations with compiler warnings/errors directly associated with the offending line(s) in the PR itself.

In this case, I had to manually instantiate it. I'm not sure if this is relevant.

what do you mean by "had to manually instantiate it"?

motoros2_interfaces__msg__MotionReadyEnum__NOT_READY_NOT_CONT_CYCLE_MODE are not defined in the current release(s) but are used

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.

Current HEAD of MotoROS2 can't be built with the M+ libmicroros distribution .zips we have made available until we release updated distribution .zips. From-source builds are essentially broken right now.

This ties into the discussion around how we version, build and distribute M+ libmicroros.

The sort of breakage we discovered here being one of the reasons we're looking into this.

ted-miller commented 4 months ago

what do you mean by "had to manually instantiate it"?

CI didn't run automatically. There was a button to make it run. I clicked it.

Current HEAD of MotoROS2 can't be built with the M+ libmicroros distribution .zips

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)

gavanderhoorn commented 4 months ago

CI didn't run automatically. There was a button to make it run. I clicked it.

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.

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)

Technically we only 'release' M+ libmicroros with each release of MotoROS2, so you could say that we're allowed to break main.

For now, users building from source can't build HEAD, only 0.1.2 or earlier.