ubi-agni / tactile_toolbox

software to handle tactile sensors in ROS
9 stars 5 forks source link

Renaming tactile_merger to avoid confusion #3

Open guihomework opened 4 years ago

guihomework commented 4 years ago

As mentioned in https://github.com/ubi-agni/tactile_toolbox/pull/2#discussion_r466383247 there has been confusion between real _tactilestate merging occurring in the tactile_state_publisher and merging of group of tactile values producing a _contactstate done in the tactile_merger

Suggested names for the package are

Renaming will affect several external files to this package and this issue will help synchronize the name switch everywhere.

guihomework commented 4 years ago

another idea would be to split into '''contact_stateconverter''' and '''contact*_clustering'''. I see at least 3 use cases like

rhaschke commented 4 years ago

I fully agree that these use case make sense and we should find a modular way to implement them. However, I don't yet like the proposal to split the merger into two separate nodes, because this essentially adds even more delay due to the additional topic. I can imagine two approaches:

  1. Implement clustering plugin to realize the different uses cases as part of the merger
  2. Use nodelets to avoid the communication overhead
rhaschke commented 4 years ago

Another reason to not split off a contact_state_converter is that the 3D position/normal information added is rather static: There is no need to publish all this information because any clustering node could read this static info from urdf equally well.

guihomework commented 4 years ago

thanks for your comments, I can understand the "delay", but there is also the "overhead" to redo something several time (adding this static information that I told about in some other email).

Each clustering needs this additional static info as well as when ones want to go straight to the tactile_pcl for publishing points for each taxel without clustering (not only per group)

adding and transmitting this info seems to augment the data transmitted over topics, but gives user access to those without the need to re-add them in their local code, and reparse the urdf etc... (they might want to do something else with this data than clustering).

using nodelets could be a solution but I am not sure one can than easily access the augmented data with rostopic echo to introspect those, and we would need to make the tactile_pcl also use nodelets then.

not sure how to proceed.

rhaschke commented 4 years ago

There is no need to make tactile_pcl a nodelet-based app. I'm only talking about your proposed contact_state_converter and contact_*_clustering nodes that should be joined in a single nodelet-based app. I cannot really follow your overhead argument adding the static information several times. Reading the information from urdf is a one-time (initialization) task and we could provide a library function to do so. Using that information in all of the clustering methods to actually compute and output that information needs to be done anyway (and doesn't add computation costs).

guihomework commented 4 years ago

If one wants to go directly to tactile_pcl with contact_states for each taxel, one needs contact_state_converter + a contact_donothing_clustering to have the data out on a topic (not on the nodelet internal topic/pointer), that then can go to tactile_pcl isn't it ?

regarding overhead, I thought that accessing the pos/ori info through a local initialized storage was some overhead when it would be there directly in the incoming _contactstates.msg AND it permits to have this data there for the user. If I am not mistaken, the rviz_tactile_state plugin also needs to read the _robotdescription to also store locally this info, and use it to compute the position in the current reference frame. We cannot avoid the transforms (in rviz_tactile_plugin, or tactile_pcl) but one could circumvent storing (in different structures) this pos/ori

I really see the point about delays, and maybe overhead on the data in topics, but _jointstates for instance stores _jointnames in each message (when it could be also loaded from some yaml) that are sometimes more bytes than the payload itself if the joint name is long. So payload size is not the biggest deal here, leaving delays the only potential problem.

When would those delays come into play ?

maybe in ROS2 all transmissions are unified and controllers chainable, so that one could use a clustering method at a slower 100 Hz as we currently code it.

What we want to tackler here is only the visual and recording. I don't think delays is an issue.

guihomework commented 4 years ago

Another point to note, a little linked to the delay you mention, is that the current design of the tactile_merger has an independent publisher, at a user selected rate, because the node "accumulates" _tactilestate info from different tactile sensors, and publishes the _contatstate msgs only at the publisher rate. Even if the timestamp is kept, there is delay when the data arrives on the topic too.

my idea would be to really just "augment" the _tactilestate information into a contactstate information with the pos/ori, also without changing the timestamp and just converting the data as it comes in. The "accumulation" of message would still be part of the `contact*_clustering`.

so I am not sure what is worse : intermediate node (one more subscriber/publisher) or de-synchronization with the separate publisher.

rhaschke commented 4 years ago

For me, it feels like more (computation) overhead when you need to pack and unpack an additional message instead of accessing an internal data structure storing the position + normal information. I guess you are more arguing about overhead for the programmer. However, this is another level of complexity, which can be easily hidden behind some nice API. To summarize, I'm still preferring a plugin-based approach to handle the grouping/clustering by a specific plugin and then average position and normal vectors withing those groups (which might comprise a single taxel only).

guihomework commented 4 years ago

Okay. then let's go for a "plugin" method. I see that switching to a nice pluginlib system will take me more time than a small switch case now. For tomorrow, we barely need a second function getRawContacts instead of getContacts (that does the clustering) here https://github.com/ubi-agni/tactile_toolbox/blob/melodic-devel/tactile_merger/src/merger_node.cpp#L61

I think it is extremely easy for now, but not for long term.