eCAL::Measurement: New methods #1232
Conversation
…formation as atomic operation.
|
@FlorianReimold please review carefully changes regarding eCAL Rec. Both commits can be reviewed separately, too, this decreases the complexity of the review. |
There was a problem hiding this comment.
I carefully reviewed the eCAL Rec part. Currently, I think the implementation would break the descriptor database, as the encoding is not used in it anymore. Therefore, std + string and proto + string would be treated equally and overwrite each others descriptors. We need to make sure, that we treat the encoding and typename as atomic pair and never use one part without the other. So basically as it was before.
| @@ -32,6 +32,24 @@ namespace eCAL | |||
| namespace eh5 | |||
| { | |||
|
|
|||
| // To be removed soon-ish! | |||
There was a problem hiding this comment.
Hmmmm, this is public API, so a comment like that for a new function feels wrong
| @@ -36,6 +36,21 @@ namespace eCAL | |||
| { | |||
| namespace eh5 | |||
| { | |||
| /** | |||
| * To be removed soon-ish | |||
There was a problem hiding this comment.
Hmmmm, this is public API, so a comment like that for a new function feels wrong
There was a problem hiding this comment.
It will be removed before we release, but as of now I could just split againeh5_writer.h in implementation & header, so creating a cpp file, and moving it over there.
| @@ -114,7 +119,7 @@ namespace eCAL | |||
| } | |||
| } | |||
|
|
|||
| if (!topic.ttype().empty()) | |||
| if (!topic_info.name.empty()) | |||
There was a problem hiding this comment.
This now must also include include the encoding. Currently, std:string would be equivalent to proto:string and flatbuffers:string.
Name and encoding must be treated atomically here, just as it was before, when they were fused as encoding:typename. Neither the encoding, nor the typename make any sense without the other one.
There was a problem hiding this comment.
Not quite. In theory, we need to consider topic, encoding and descriptor to be atomic. Either you have all 3 information, or none. We sometimes might have cases where we have the usecase where we have 2 subscribers, and the info distinguishes only in the descriptor. However, to each publisher / subscriber, all 3 info are atomic.
We might have to change this method to account for the this fact.
There was a problem hiding this comment.
Well yeah, ok, but not even treating encoding and typename as an atomic pair seems to head in the different direction. The goal of this code is to enrich the measurement with a descriptor, even though it may not be sent by some node.
| } | ||
| } | ||
| } | ||
|
|
||
| // Update the type_descriptor_map (can of course only work if we have the type information available) | ||
| if (!topic.ttype().empty()) | ||
| if (!topic_info.name.empty()) |
There was a problem hiding this comment.
Again, now when we have the encoding and type separated, we have to actively make sure, that they are treated as an atomic pair that doesn't behave separated.
The descriptor must always be asigned to a encoding + typename pair.
| { | ||
| int quality_for_other_channels = (this_topic_info_quality & ~INFO_COMES_FROM_CORRECT_TOPIC_QUALITYBIT); | ||
|
|
||
| auto type_descriptor_map_it = type_descriptor_map.find(topic.ttype()); | ||
| auto type_descriptor_map_it = type_descriptor_map.find(topic_info.name); |
There was a problem hiding this comment.
Again, now when we have the encoding and type separated, we have to actively make sure, that they are treated as an atomic pair that doesn't behave separated.
The descriptor must always be asigned to a encoding + typename pair.
| std::map<std::string, std::pair<int, std::pair<std::string, std::string>>> channel_descriptor_map; // ChannelName -> {Type, Descriptor} | ||
| std::map<std::string, std::pair<int, std::string>> type_descriptor_map; // Type -> Descriptor (used for topics that we know the type of, but have no other information available) | ||
| std::map<std::string, std::pair<int, eCAL::SDataTypeInformation>> channel_descriptor_map; // ChannelName -> {Type, Encoding, Descriptor} | ||
| std::map<std::string, std::pair<int, eCAL::SDataTypeInformation>> type_descriptor_map; // Type -> DatatypeInformation (used for topics that we know the type of, but have no other information available) |
There was a problem hiding this comment.
This must now be a map of [enconding + typename] -> DataTypeInformation. The descriptor information will always stick to a pair of those, as e.g. std + string and proto + string will have very different descriptors, just as it was when they were still fused together as std:string and proto:string.
Neither the encoding, nor the typename must exist without the other, so now that we have them separated, we must actively make sure to never use one without taking care of the other as well.
I even think, that storing the Descriptor as std::string is totally fine, here. The SDataTypeInformation will only hold redundant information anyways.
There was a problem hiding this comment.
For the second point, yes I know it now holds redundant information, but it's easier to use, as you can just extract the datatype information.
The whole point of this PR is to treat all this info (SDatatypeInformation) as atomic.
Btw, is this even a valid usecase: typename but no descriptor? Or descriptor is updated after typename?
To be honest, I don't think it is. So maybe we can even get rid of the second map?
There was a problem hiding this comment.
The reason of this code is to address the fact, that typename and descriptor are not atomic, i.e. there may be different combinations that I must choose from.
There may be user code that doesn't send descriptor information. So I know the type of a topic, but don't have a descriptor. Another node may however publish it, so I take it from there.
A second use case is that there may be multiple different descriptor sent for the same type, e.g. from the publisher and subscriber. So I need to choose one. In a world where people edit their proto files from time to time, this is a very realistic situation.
Of course, if you make the encoding + name atomic again, you can simply store the entire info struct. It may contain redundant information, but those also don't hurt.
| if (type_descriptor_map_it == type_descriptor_map.end()) | ||
| { | ||
| // Save the new descriptor | ||
| type_descriptor_map.emplace(topic.ttype(), std::make_pair(quality_for_other_channels, topic.tdesc())); | ||
| type_descriptor_map.emplace(topic_info.name, std::make_pair(quality_for_other_channels, topic_info)); |
There was a problem hiding this comment.
Again, now when we have the encoding and type separated, we have to actively make sure, that they are treated as an atomic pair that doesn't behave separated.
The descriptor must always be asigned to a encoding + typename pair.
I also don't see how storing the topic_info instead of just the descriptor string provides a benefit here.
| topic_info_map_entry.second.description_quality_ = channel_descriptor_entry_it->second.first; | ||
| } | ||
|
|
||
| if (!topic_info_map_entry.second.type_.empty()) | ||
| if (!topic_info_map_entry.second.tinfo_.name.empty()) |
There was a problem hiding this comment.
Again, now when we have the encoding and type separated, we have to actively make sure, that they are treated as an atomic pair that doesn't behave separated.
| { | ||
| auto type_descriptor_entry_it = type_descriptor_map.find(topic_info_map_entry.second.type_); | ||
| auto type_descriptor_entry_it = type_descriptor_map.find(topic_info_map_entry.second.tinfo_.name); |
There was a problem hiding this comment.
Again, now when we have the encoding and type separated, we have to actively make sure, that they are treated as an atomic pair that doesn't behave separated.
|
Ok, yes, it's incorrect to "just" use the type, when you need to consider E.g. only store the Datatypinfo (with the quality bit), and update the whole thing if a higher quality is available? The "name" is not the best thing to connect the topicInfo to. Better would be to connect it to Publisher / Subscriber Ids. But that would require at least the same amount of work like introducing SDataTypeInformation. |
Not sure how you would simplify the logic. You still need to choose a descriptor for an encoding + typename. I expect the new format to lead to a slightly more complicated solution, as you need to fuse encoding and typename back together and choose a descriptor based on both.
I don't really understand this, sorry. |
|
I will make a new PR which needs to be merged before this one. |
Description
Atomic operation for getting / setting datatype information
Related issues
Preparation for #1076