refactor(LibCarla, Carla): rename DDS middleware abstraction to Middleware - #9711
Conversation
|
Hello! It could work. But in my opinion, some names are not explicit enough in context. You changed What is inside the folder |
|
I added some inline code suggestions with the changes I think you should make to provide a clearer ROS2 context for this refactoring in the project. You also should rename the file And probably update the text description in the CHANGELOG.md. |
|
Thanks @JArmandoAnaya, very helpful feedback! I have pushed new commit with all 10 suggestions applied, the file rename, and the CHANGELOG update. |
|
Hi @LuisPovedaCano , this PR has been reviewed by @JArmandoAnaya . Could you help merge when you have a moment? Thanks! |
LuisPovedaCano
left a comment
There was a problem hiding this comment.
Thanks for the clean rename.
I've left a requested change inline, please address it before merge.
|
The smoke test failure looks unrelated — it's a physics determinism test, and all ROS 2 tests passed. Could you re-run the failed CI? Thanks! |
|
I already tested locally on top of the current |
Description
This PR drops the
DDSprefix from the ROS 2 middleware abstraction layer:dds/→middleware/IDDSPublisher/SubscriberMiddleware→IPublisher/ISubscriberMiddlewareDDSMiddleware→MiddlewareDDSMiddlewareFactory→MiddlewareFactoryCARLA_ROS2_DDS_*→CARLA_ROS2_MIDDLEWARE_*UCarlaSettings::DDSMiddlewareName→MiddlewareNametest_dds_middleware.cpp→test_middleware.cppI have read #9692. The DDS-prefixed names were kept on purpose, and future non-DDS RMWs (like Zenoh) are meant to live alongside DDS, not replace it.
However, when I looked at how a non-DDS RMW would actually fit in, I saw a problem in PublisherImpl.h (and the matching SubscriberImpl.h). The base publishers that use this template are already generic — they don't care which backend is used. But inside
PublisherImpl<T>, the_middlewarefield is typed asIDDSPublisherMiddleware, which ties it to DDS.So a non-DDS RMW has only two ways to fit in:
PublisherImpl<T>for its own family (e.g.ZenohPublisherImpl<T>holding anIZenohPublisherMiddleware), and update every high-level publisher (CarlaImagePublisher,CarlaLidarPublisher, …) to pick the right one. Same again for subscribers. That's a lot of duplication.ZenohPublisherinheritIDDSPublisherMiddlewaredirectly. No duplication, but the name doesn't match — a Zenoh class implementing a DDS-named interface, and aDDSMiddlewareenum that needs aZenohmember.Looking at IDDSPublisherMiddleware itself — its methods are
Init,Publish,IsAlive,GetTopicName— none of these are DDS-specific; they describe what any publisher needs to do. So a third way is to drop theDDSprefix fromIDDSPublisherMiddleware/IDDSSubscriberMiddleware(and the related enum / factory). A new family can then implementIPublisherMiddlewaredirectly and reusePublisherImpl<T>and the base publishers without changes. No code duplication, no name mismatch. To me this fits the "alongside" idea more naturally, and that's what this PR does.For context, our next step is a separate PR adding a Zenoh implementation under
middleware/zenoh/, alongsidefastdds/andcyclonedds/. This rename clears the way forZenohPublisherMiddlewareto inheritIPublisherMiddlewaredirectly.@JArmandoAnaya — since this builds on the DDS abstraction you have introduce, would appreciate your thoughts on the naming.
Where has this been tested?
I have tested both build and launch locally — both work fine.
This change is