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.

Sign up for GitHub

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

Jump to bottom

Raise alarm if calib data can't be loaded on multi-group systems with TF enabled #169

Merged
merged 6 commits into from
Mar 1, 2024
Merged

Raise alarm if calib data can't be loaded on multi-group systems with TF enabled #169

merged 6 commits into from
Mar 1, 2024

Conversation

gavanderhoorn
Copy link
Collaborator

@gavanderhoorn gavanderhoorn commented Oct 3, 2023 •
edited
Loading

But only if it hasn't been explicitly disabled by the user.

Should fix #54.

For the discussion: see also #54.

Haven't tested it yet.

@gavanderhoorn gavanderhoorn added this to the 0.1.2 milestone Oct 3, 2023
@gavanderhoorn

This comment was marked as resolved.

Sign in to view
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch from c640769 to 089c25e Compare October 3, 2023 10:14
@gavanderhoorn gavanderhoorn mentioned this pull request Oct 3, 2023
Closed
@gavanderhoorn gavanderhoorn changed the title refactor: new function to load calib data Raise alarm if calib data can't be loaded on multi-group systems with TF enabled Oct 3, 2023
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch 2 times, most recently from 85c9c6e to ae5018a Compare October 3, 2023 13:41
@gavanderhoorn
Copy link
Collaborator Author

Tested, works:

Ros_Controller_Initialize: calib loaded ok: 0, should warn: 1
Ros_Controller_Initialize: no calibration files loaded: TF potentially incorrect -> warn (disable with 'disable_missing_calib_warning')

on a YRC1 with a GP25 on a track without calibration.

@ted-miller
Copy link
Collaborator

on a YRC1 with a GP25 on a track without calibration

This comment got me thinking. I think I went about the base-track-tf all wrong. I don't think that base tracks use the same calibration method as multi-robot and station-axes.

Do you even have a Robot Calib or Grp Combination button on your pendant?

image

image

I think the base track uses different parameters for its calibration. It's basically an implied group combination.

If you jog your track, does your /tf visualization move with it?

Assuming I'm right about the base calibration (or lack thereof), then this alarm will always occur on a system with a base. Also, it would mean that /tf is never going to work right.

@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Oct 4, 2023 •
edited
Loading

Do you even have a Robot Calib or Grp Combination button on your pendant?

nope.

If you jog your track, does your /tf visualization move with it?

nope.

Also, it would mean that /tf is never going to work right.

with the current implementation I guess? What's needed would be to broadcast the current Yaskawa Base to Yaskawa RF IIUC? Or at least add it to the world -> base (non-Yaskawa base that is) transform we're broadcasting now.

@gavanderhoorn gavanderhoorn mentioned this pull request Oct 4, 2023
Closed
@gavanderhoorn
Copy link
Collaborator Author

Opened #170 to track the TF-is-incorrect-for-robots-on-tracks issue.

@ted-miller
Copy link
Collaborator

Also, it would mean that /tf is never going to work right.

with the current implementation I guess?

Yeah.

What's needed would be to broadcast the current Yaskawa Base to Yaskawa RF IIUC?

The calibration from BF to RF exists. It's just not in the mpGetCalibration data. I think it's in SCG parameters.

ted-miller
ted-miller reviewed Oct 5, 2023
View reviewed changes
src/ControllerStatusIO.c Outdated Show resolved Hide resolved
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch from ae5018a to 02d3698 Compare October 10, 2023 19:16
@gavanderhoorn gavanderhoorn marked this pull request as draft October 10, 2023 19:17
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch from 02d3698 to 7baca00 Compare October 13, 2023 16:25
@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Oct 13, 2023 •
edited
Loading

I believe this should now work.

In essence, this checks whether there are groups which don't have an associated base group and if there are, and if no calibration files loaded, TF is enabled and the warning is not suppressed, it raises the alarm.

In all other cases it stays silent.

On my configuration this prints:

Ros_Controller_Initialize: calib loaded ok: no, should warn: no

@gavanderhoorn
Copy link
Collaborator Author

@ted-miller: could you perhaps test this on some more complex group configurations?

@gavanderhoorn gavanderhoorn marked this pull request as ready for review October 13, 2023 16:29
@ted-miller
Copy link
Collaborator

Sure. It'll be Monday or Tuesday. I've currently got everything configured for single-robot.

@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch 2 times, most recently from 0e882c3 to 9b37337 Compare October 16, 2023 12:22
@gavanderhoorn
Copy link
Collaborator Author

Friendly ping @ted-miller.

@ted-miller
Copy link
Collaborator

So, with R1+R2+S1, if I don't have any calibrations, I get the alarm. If I then add a cal for R2+S1 and reboot, there is no alarm.

ted-miller
ted-miller requested changes Oct 20, 2023
View reviewed changes
src/ControllerStatusIO.c Outdated Show resolved Hide resolved
@gavanderhoorn gavanderhoorn modified the milestones: 0.1.2, untargeted Nov 8, 2023
@ted-miller ted-miller mentioned this pull request Nov 15, 2023
Closed
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch from 69fdf55 to 2b7a861 Compare November 23, 2023 08:40
@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Nov 23, 2023 •
edited
Loading

@ted-miller: I've updated the PR with the changes from the branch I mentioned in #169 (comment). I've also included the changes to address your review comments from #189.

ted-miller
ted-miller reviewed Nov 27, 2023
View reviewed changes
src/ControllerStatusIO.h Outdated Show resolved Hide resolved
src/ControllerStatusIO.c Show resolved Hide resolved
src/ControllerStatusIO.c Outdated Show resolved Hide resolved
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch 2 times, most recently from fd25ac8 to a16bcf3 Compare February 2, 2024 13:37
@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Feb 2, 2024 •
edited
Loading

@ted-miller: I believe this addresses your latest feedback.

I've tested it on my controller, and it appears to work -- as in: I don't get an alarm with just my base axis, even though I have not disabled the alarm nor have I any calibration file(s).

Could you perhaps try this on your setup?

@ted-miller
Copy link
Collaborator

My multi-group setup is being used for testing something else. It should free up later this week.

@gavanderhoorn
Copy link
Collaborator Author

My multi-group setup is being used for testing something else. It should free up later this week.

you guys do other things beside maintaining/developing MotoROS2?

PS: running it on a single-group system would also be a good test.

@ted-miller
Copy link
Collaborator

you guys do other things beside maintaining/developing MotoROS2?

Of course. We're scoping out the framework for MotoROS3. What else would we be doing?

@gavanderhoorn
Copy link
Collaborator Author

Friendly ping @ted-miller :)

@ted-miller
Copy link
Collaborator

Still on the todo list. Haven't forgotten this.

@gavanderhoorn
Copy link
Collaborator Author

Friendly ping @ted-miller :)

@ted-miller
Copy link
Collaborator

Still on the todo list. Haven't forgotten this.

Ok... I did forget this.
I won't get to it today. But I should be able to this week.

ted-miller
ted-miller approved these changes Mar 1, 2024
View reviewed changes
Copy link
Collaborator

@ted-miller ted-miller left a comment

Choose a reason for hiding this comment

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

I have tested on R1R2S1 and everything looks good.

No calib with ignore_missing_calib_data: false = alarm
Calib with ignore_missing_calib_data: false = no alarm
No calib with ignore_missing_calib_data: true = no alarm

Do you want to merge with the tests disabled? Or should we look at fixing those?

@gavanderhoorn
Copy link
Collaborator Author

I don't really have time to figure out why the tests crash the controller. I have a hunch it's stack related, but haven't been able to check it.

I would be OK with merging without the tests, as I have been able to run them by disabling the other tests. They passed.

As it's been a while, I'd like to rebase this and then merge.

@gavanderhoorn
refactor: new function to load calib data
b14dab6
@gavanderhoorn
Add 'missing calibration warning' cfg item
321c81a 
Allow user to disable raising an alarm in case a multi-group system has
not been calibrated, but TF broadcast is still enabled (in which case
group origins will be wrong).
@gavanderhoorn
Raise alarm in case none of the calib files loaded
e7fa32f 
Note: as mentioned in the comments, it's perfectly OK for multi-group
systems to not have been calibrated. Whether or not calibration
succeeded to load is therefore not taken into account when determining
whether controller initialisation was successful or not (`bInitOk`).

But, in case TF broadcasts are enabled, uncalibrated groups
(specifically: robots) will have their origins coincident, which will
(most likely) not be representative of their physical configuration (ie:
in case those groups are different manipulators, those will not occupy
the same physical space).

In those cases an alarm is raised to notify the user of this potential
misconfiguration.

But only if the alarm hasn't been disabled by user configuration.
@gavanderhoorn
Document new alarm (8013[16])
13dd95a
@gavanderhoorn
Add tests for ShouldWarnNoCalibDataLoaded(..)
42a31cb 
Initial set to encode assumptions.
@gavanderhoorn
Disable ctrlr-crashing tests (for now)
0368ad8 
Need to diagnose the crashes.
@gavanderhoorn gavanderhoorn force-pushed the warn_about_missing_calib branch from a16bcf3 to 0368ad8 Compare March 1, 2024 18:35
@gavanderhoorn
Copy link
Collaborator Author

Oh, right. CI is broken (#212 (comment)).

Well, it builds, I promise 🤞

@ted-miller: ok with the merge?

@ted-miller ted-miller merged commit ee8844f into main Mar 1, 2024
5 of 14 checks passed
@ted-miller ted-miller deleted the warn_about_missing_calib branch March 1, 2024 18:56
@gavanderhoorn gavanderhoorn modified the milestones: untargeted, 0.1.3 Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ted-miller ted-miller ted-miller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1.3
Development

Successfully merging this pull request may close these issues.

Warn about failure to retrieve multi-robot calib data
2 participants
@gavanderhoorn @ted-miller