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 support for INFORM job CRUD #278

Open
4 tasks
gavanderhoorn opened this issue Jul 15, 2024 · 9 comments
Open
4 tasks

Add support for INFORM job CRUD #278

gavanderhoorn opened this issue Jul 15, 2024 · 9 comments
Labels
enhancement New feature or request help wanted roadmap This is an item on the roadmap todo Not an issue, just a TODO

Comments

@gavanderhoorn
Copy link
Collaborator

gavanderhoorn commented Jul 15, 2024

From the roadmap:

The following items are on the MotoROS2 roadmap, and are listed here in no particular order:

  • [..]
  • CRUD of INFORM job files (ie: create, retrieve, update, delete)
  • [..]

In addition to the listed functionality, we'd also need to be able to list existing jobs, as the other operations will need names of jobs to operate on.

The goal of such services would be to allow a ROS 2 application to completely manage robot native INFORM jobs. Coupled with INFORM job coordination services, this would allow a ROS 2 application to use MotoROS2 to create, retrieve, update, delete, start and stop INFORM jobs.

ROS-based motion planning could be used to bring a robot (/EEF) to a specific location where an INFORM job could take over and perform some specific process (such as welding, using the Yaskawa provided support for welding).

In summary: it would make hybrid ROS 2 + INFORM applications possible.

Progress:

@gavanderhoorn gavanderhoorn added enhancement New feature or request help wanted todo Not an issue, just a TODO roadmap This is an item on the roadmap labels Jul 15, 2024
@gavanderhoorn
Copy link
Collaborator Author

Current design of the services: here (diff).

@gavanderhoorn
Copy link
Collaborator Author

@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Aug 13, 2024

I've been trying to test my latest changes (adding 'the rest' of the CRUD operations), but for some reason none of the new services 'work'. The client just hangs. No output on the server side (in the debug log).

Almost as-if the executor isn't processing the events. Removing the list service (ie: one that definitely works) from the executor results in the same behaviour (of that service).

I've compared the code (diff) to other service additions, but can't really find anything incorrect.

I'm probably missing something. @ted-miller: could you maybe take a look and see whether you can spot anything?

@ted-miller
Copy link
Collaborator

It seems that the buffer for the incoming name field needs to be allocated in Ros_ServiceGetInformJob_Initialize (and other init functions)

    rosidl_runtime_c__String__init(&g_messages_GetInformJob.request.name);
    rosidl_runtime_c__String__assign(&g_messages_GetInformJob.request.name, "this is a string to allocate a buffer");

I'm not really sure why assign was also needed. But it worked after I added that.

@gavanderhoorn
Copy link
Collaborator Author

gavanderhoorn commented Aug 14, 2024

Ah, right ☕ 😪


Edit: switched to using micro_ros_utilities instead of doing this all manually: 2bc333f.

@gavanderhoorn
Copy link
Collaborator Author

The latest additions now work, but there's one thing to consider/discuss: I initially configured a maximum job file size of 4 KB. That turns out to be way too small, so I increased it to 64 KB.

But, as we need / want to preallocate all resources for all services, this means we're preallocating 128 KB just for the get and put services.

Total memory usage on my YRC1 is now approximately 220 KB.

@gavanderhoorn
Copy link
Collaborator Author

@ted-miller: could you perhaps check what a useful maximum upper limit for job files could be? I don't have access to anything representative as I don't normally use INFORM.

@ted-miller
Copy link
Collaborator

The biggest job I have locally on my PC is 28 KB.

But, as we need / want to preallocate all resources for all services, this means we're preallocating 128 KB just for the get and put services

I'm not certain that we need to do that. In my example above, I use the string "this is a string to allocate a buffer". But the job I requested was much larger than that and the whole thing was transferred. So, it's possible that the executor is reallocating the buffer as needed. (And it's also possible that I just had a massive buffer overflow and overwrote a bunch of memory inadvertently.)

@gavanderhoorn
Copy link
Collaborator Author

So technically you're correct. We don't have to pre-allocate. However, the micro_ros_utilities(..) functions I now use (2bc333f) do pre-allocate, and they will only do that based on the size configured for those fields.

We would have to revert to manually managing the request and reply fields, but:

  • that would still leave us with the request: it has to be pre-allocated or micro-ROS won't be able to receive messages (and dynamic allocation has been disabled)
  • we wouldn't be able to say anything about the maximum memory usage (bigger jobs retrieved at a later time than initialisation could lead to late user-facing out-of-memory errors)

But the job I requested was much larger than that and the whole thing was transferred. So, it's possible that the executor is reallocating the buffer as needed. (And it's also possible that I just had a massive buffer overflow and overwrote a bunch of memory inadvertently.)

I would expect the latter to have happened.

I don't believe assign(..) checks anything (that would be assignn(..)'s job).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted roadmap This is an item on the roadmap todo Not an issue, just a TODO
Projects
None yet
Development

No branches or pull requests

2 participants