Issue-549: Moved Kafka time producer from CDASimAdapter to CARMA-Stre… #550
Conversation
…ets plugin + Update CARMA-Streets Plugin to be simulation time aware
…d Pure Virtual error
+Rule of 5 for Kafka consumer and producer + Remove virtual destructor for CARMAStreetsPlugin
| clock = std::make_shared<CarmaClock>(simulationMode); | ||
| if (simulationMode) { | ||
| clock = std::make_shared<CarmaClock>(_simulation_mode); | ||
| if (_simulation_mode) { |
There was a problem hiding this comment.
Do we need the if statement to only apply filter when it is in simulation mode? Is it possible for CDAAdapterPlugin to broadcast TimeSyncMessage when it is not in simulation mode?
There was a problem hiding this comment.
No it is not possible and also not valid. TimeSync messages are only valid when we are running in simulation. Otherwise we use machine time.
There was a problem hiding this comment.
AddMessageFilter<tmx::messages::TimeSyncMessage>(this, &PluginClientClockAware::HandleTimeSyncMessage);
The AddMessageFilter is event based and should only be triggered when there is Timsync message sent to the TMX bus in simulation mode.
so the _simulation_mode should always be true. Can we remove this if check statement? The same to the HandleTimeSyncMessage() function. There is no need to check if _simulation_mode true.
There was a problem hiding this comment.
That is incorrect. Simulation mode will be false in real world deployments. Here the PluginClockAwareClient will not subscribe to TimeSync messages so this is valid in my opinion
There was a problem hiding this comment.
To clarify, the CDASimAdapter will only be active in simulation mode, which means we will only broadcast TimeSync messages in Simulation Mode. The PluginClockAwareClient is a PluginClient that is able to configurably use Simulation time or real time. It will only subscribe to TimeSync messages when in simulation mode. So on the broadcast side the plugin will not be active to produce time sync messages if not in simulation. On the filter side, even if something else did produce a time sync message, we will not add a filter for it if not in simulation mode.
| virtual ~kafka_producer_worker(); | ||
| // Rule of 5 because destructor is define (https://www.codementor.io/@sandesh87/the-rule-of-five-in-c-1pdgpzb04f) | ||
| // delete copy constructor | ||
| kafka_producer_worker(kafka_producer_worker& other) = delete; |
There was a problem hiding this comment.
What is the purpose of deleting the copy and move constructor?
There was a problem hiding this comment.
Rule of 5 is a rule in C++ that if you define one of these you need to explicitly define all to avoid issues with compiler generated versions. This is to address a Code Smell. (https://www.codementor.io/@sandesh87/the-rule-of-five-in-c-1pdgpzb04f)
There was a problem hiding this comment.
I left a comment as well
…ets plugin
PR Details
Description
Move Kafka time producer for CARMA-Streets time synchronization to CARMA-Streets plugin. This removes unnecessary dependency on kafka for CDASimAdapter and also makes CARMA-Streets Plugin contain entire interface to CARMA-Streets including simulation. CARMA-Streets Plugin now extends our simulation time aware PluginClient which can be further extended to listen for simulated messages.
Related Issue
#549
VH-1237
Motivation and Context
How Has This Been Tested?
Locally integration testing with python time step script to send udp time step messages.
Types of changes
Checklist:
V2XHUB Contributing Guide