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

Add INFORM job list service #279

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

gavanderhoorn
Copy link
Collaborator

@gavanderhoorn gavanderhoorn commented Jul 15, 2024

Work towards #278.

This implements a basic job/list function which returns the names of all jobs (*.JBI) on the controller.

I've submitted this as a small PR to avoid opening something for all 5 services at once. It is fully functional though, hence this is not a draft.

There are a couple of TODOs which I'd like to discuss.

Note: this depends on new messages in motoros2_interfaces: Yaskawa-Global/motoros2_interfaces#21.

@gavanderhoorn
Copy link
Collaborator Author

Note: CI will fail as there is no build of micro_ros_motoplus with the changes in Yaskawa-Global/motoros2_interfaces@main...gavanderhoorn:motoros2_interfaces:inform_crud.

src/ServiceJobList.c Outdated Show resolved Hide resolved
src/ServiceJobList.c Outdated Show resolved Hide resolved
src/ServiceJobList.c Outdated Show resolved Hide resolved
src/ServiceJobList.c Outdated Show resolved Hide resolved
src/ServiceJobList.c Outdated Show resolved Hide resolved
src/RosApiNameConstants.h Outdated Show resolved Hide resolved
src/ErrorHandling.h Outdated Show resolved Hide resolved
src/RosApiNameConstants.h Outdated Show resolved Hide resolved
@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Jul 17, 2024

I've addressed most of the discussed changes in functionality. Tested it here as well and it still works.

Just need to test behaviour when there are more jobs than allowed, define proper result_codes and document it all.


Edit:

need to test behaviour when there are more jobs than allowed

this case is handled correctly.

@ted-miller: I first thought to always return the first N jobs, but use a non-success result_code in case more jobs than 'allowed' exist, but in the end I've changed the implementation to just return a specific failure result_code and just return an empty list.

doc/ros_api.md Outdated Show resolved Hide resolved
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 don't see any issues. But I'm struggling to build right now.

So, If you're happy and have tested it, then feel free to merge. Otherwise, I'll get my stuff fixed on monday and actually run it.

@gavanderhoorn
Copy link
Collaborator Author

I have tested, but I'd like some independent confirmation I'm not making (implicit) assumptions about how things work based on my controller configuration.

I'll make a build of micro_ros_motoplus available to you with the needed interfaces.

@ted-miller
Copy link
Collaborator

I have fixed my build.

ted-miller
ted-miller previously approved these changes Jul 22, 2024
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.

Works well. I also verified that it lists macro jobs and system jobs.

Japanese names come across as gibberish. But this is to be expected. (Yaskawa-Global/motoros2_interfaces#21 (comment))

@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Jul 23, 2024

@ted-miller: I believe this is the final version of this PR.

Switched to named result codes and strings, renamed files to match the fact it's now specifically about INFORM jobs and updated all references.

If you could take a last look?

To make it easier: 20240723_mr2_yrc1_h_inform_list_rc2.7z.zip - removed (it's not a .zip).

This should not require any updates on the motoros2_interfaces side, but just in case: I updated Yaskawa-Global/motoros2_interfaces#21.

@gavanderhoorn
Copy link
Collaborator Author

I'll likely squash some of the commits before merging.

@ted-miller
Copy link
Collaborator

I'll give this a run first-thing tomorrow

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.

Looks good. Tested on YRC. But since you're using vanilla M+ API's, I'm not worried about other controllers.

src/ServiceInformJobList.c Show resolved Hide resolved
As implied by the name of the service, this allows clients to retrieve a list of INFORM jobs (JBI) on the controller.
Move away from magic numbers and strings.
@ted-miller
Copy link
Collaborator

@gavanderhoorn Do you want to merge this, or pull inform_job_crud_the_rest into it first?

@gavanderhoorn
Copy link
Collaborator Author

I'd like to take another look at how this reports errors to the client. I just haven't found time to do that yet.

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