micro-ROS / micro_ros_diagnostics

Diagnostics framework for micro-ROS
Apache License 2.0
8 stars 4 forks source link

Support multiple HW id per updater #16

Closed bjv-capra closed 2 years ago

bjv-capra commented 3 years ago

As a user, I'd like to be able to utilize the same updater to send information from different hardware devices.

As a use case, I have a board that collects information coming in from N different peripherals devices. This board is my micro-ROS client and runs a diagnostic_updater. Each peripheral may have up to M valuable information I may want to report up. Thus, I'd like to be able to also indicate to what HW_id a task belongs.

Does this use case make sense?

norro commented 3 years ago

I think that does indeed make sense. Especially in the use case you're describing.

I think currently you would have to instantiate one updater per hwid. But what you would like to have is one updater with one task per hwid, is that correct?

bjv-capra commented 3 years ago

Actually, it'd be X tasks, where a task belongs to an HW_ID and Task_ID. Maybe from one HW_ID, we have 3 tasks belonging to HW_ID 1, that is i.e: [[1,1], [1,2], [1,3]], however, I may also have HW_ID 2 with 2 tasks, i.e. [[2,1], [2,4]]. In total, there'd be 5 tasks in this case. The reason why I give these examples is that I want to reuse the task ID (although they are different). And yes, only one updater (it'll simplify the bridge).

I don't think it makes sense to force it down to one per HW_ID, as one hardware may have different data to report.

norro commented 3 years ago

So this would basically mean associating the hwid with the task and no longer with the updater, right? This should give enough freedom for the scenario you are suggesting.

Do you mind suggesting a PR for this?

bjv-capra commented 3 years ago

Actually, I still think they would be associated to the updater. I'd simply remove the HW_ID as an argument to the updater_init, and add it to the task_init. Makes sense?

norro commented 3 years ago

Sounds good

bjv-capra commented 3 years ago

Do you mind suggesting a PR for this?

I'll give it shot.

bjv-capra commented 3 years ago

I've started to look at this implementation and came to a bit of confusion. Let me clarify I haven't developed much with ROS_DIAGNOSTICS in general.

Under ros_diagnostics the hardware_id and the name from DiagnosticStatus are of the same component. While on our message definition, we have a updater_id and a hardware_id. The updater_id is later used to lookup for the name. Hence, if my understanding of this is correct, perhaps what we should do is to remove the updater_id and simply keep the hardware_id. The behavior will pretty much be the same. Besides this change, I'll move the hardware_id to the task, thus, one will init a task by providing hardware_id and value_id.

This would then translate into a map of the like of:

Hardware ID Name
0 <Component 0>
1 <Component 1>
N
Value ID Name
0 State
1 Duration
N Errors

The latter would be used for the KeyValue parameter.

We could later find a way to maybe pack all this as DiagnosticArray (rather than just DiagnosticStatus).

To wrap up, I'm pondering about the actual need of:

Thoughts?

bjv-capra commented 3 years ago

@norro friendly ping

norro commented 3 years ago

Hey @bjv-capra. To my understanding, all the different ID's exist to enable (in vanilla ROS 2)

  1. Several hardware units reporting (each with a different hardware ID), e.g., different robots or different computation units
  2. Several updaters per hardware (each with a different updater ID), e.g., one for actuators, one for sensors, one for system monitoring (cpu, memory)
  3. Several tasks per updater, e.g., each checking one sensor, actuator, ...
  4. (The additional value_id in micro-ROS diagnostics is just for the fact, that we don't want to create/send strings on the uC. With the value_id, one can encode according string/... values from the lookup table hat are then translated by the bridge.)

(this document gives a bit of an idea, how it is intended)

The current micro-ROS diagnostics mimics 1.-3. to allow for the same flexibility. From what I understand, you are suggesting to drop flexibility. Is this correct? Do you think that one of 1.-4. is not necessary or not feasible in a uC setup?

bjv-capra commented 3 years ago

I'm not meaning to remove flexibility. I'm just saying that to my understanding, we have IDs that are redundant. The HW unit in my mind is the HW reporting (i.e. motor 1). Then, we have updater_id which has a comment above indicating the name (as an ID). However, as I understood it, the HW_ID and Name(updater_id) were the same.

Maybe this is simply a miss understanding from my side wrt to how to use the updater, as I wouldn't want 1 publisher to be created per HW on an embedded system, as it'd consume too many resources. Instead, I'd like to create 1 task per component I want to report, indicating HW and Value. This approach also has its issues as it centralizes all the collection of information, however, this is how to envision could be the most optimal from a constraint point of view.

bjv-capra commented 3 years ago

I'll try to create a PR with a draft for this soon.

bjv-capra commented 3 years ago

Okay, sorry for the delay. I had to go through the documentation of Diagnostics and the RFC to better understand what you're suggesting as I was a little bit confused.

So, the current design I think, is on the correct track (except the hardware_id).

What I wanted to do, or have the ability to do is not to be forced to have multiple updaters on my embedded system, rather only one. The main reason why I wanted this, is because the call to rclc_diagnostic_updater_init consumes 375 words out of my stack (quite a bit) while creating a simple publisher only consumes 27 words. This needs to be addressed.

On the other hand, my approach would require more tasks which I believe would be less expensive than creating multiple updaters.

To sum, I think we have to add a few options to the API which I don't think to need to be exclusive

  1. A user predefines(cmake option) how many MAX updaters it will want to have (1->N). Then the user creates one updater per hw component (indicating the hw_id at the same time).
  2. A user predefines(cmake option) how many MAX tasks it may want to have (1->N).
  3. A user may have an updater to send updates for multiple devices. However, for this purpose, the updater shouldn't be init'ed with an id (nor hw id), and the task would carry the updater_id and the hw_id. This leaves a loophole where the user may assign the same ID to an updater and also pass it to a task, however, this should be up to the user to define.
norro commented 3 years ago

I think having fixed maxima of updaters and tasks via cmake is definitely fair for a uc setup. I also think that a cmake option for the hardware_id (not only it's size) could make sense, as the ID should be fix for one particular uc anyway. don't you think?

I also think that restricting ourselves to one updater (read: micro_ros_diagnostic_updater) per uc is fair. To still simulate/pretend different updaters (read: logical ROS 2 diagnostic updaters), moving the updater ID to the tasks makes sense as well.

I think, though, that we should do either 1.+.2. or 2.+3. to not be too confusing. So either define max number of updaters and tasks via cmake (so actually allowing several updaters per device) and then having the updater ID in the updater or having just one uros updater per uc and then simulate different logical updaters by moving the updater to the tasks.

bjv-capra commented 3 years ago

I can provide you a live example, we have one uc that concentrates all peripherals from our platform (coming via different protocols), each peripheral has its own hardware_id (read: serial number). Hence, I'd like to pass the hardware_id + updater_id (read: id used to lookup the internal name of the device). However, I'd like to do this utilizing the least amount of memory possible, which seemed to be having X tasks and 1 updater. This would be 2+3 and not 1+2.

norro commented 3 years ago

Sounds good to me!