Feature/carma messenger ambassador

[EricChen-Lei]

PR Details

Description

Implement carma messenger ambassador

Related GitHub Issue

Related Jira Key

CXC-88

Motivation and Context

How Has This Been Tested?

Types of changes

• Defect fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that cause existing functionality to change)

Checklist:

• I have added any new packages to the sonar-scanner.properties file
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[chengyuan0124]

Do we receive the messengerEmergencyState when the CARMA Messenger sends its registration to the CARMA Messenger Ambassador?

[chengyuan0124]

Do we still use the function name same with CARMA Ambassador or we can change to something like "carmaMessengerVehicleId"?

[kjrush]

I'm a bit concerned about the amount of duplication in this one, it looks like a lot of this is the same as the CARMA Platform ambassador, which is fine and expected. But rather than simply duplicating the code, we should be looking at how to refactor it to minimize duplication. Can we inherit directly from the CARMAPlatformAmbassador class and override the specific functions we need to change to support messenger? Are there parts of the CARMA Platform Ambassador that should be refactored out into a base-class that both Platform and Messenger ambassadors inherit from? Should some of the existing classes just be renamed and reused as-is instead of duplicated?

[kjrush]

Generally looks good to me! I've got a few minor nitpicks that you can choose to take a look at or ignore, but I think this is a good approach overall!

[kjrush]

Is this still an open todo item?

[EricChen-Lei]

Thank you Kyle, I must have missed this part, will delete it right away

[kjrush]

At runtime, will the formatting for this logger show the actual inheriting class or will all log messages now seem to come form "CommonMessageAmbassador"? Might be confusing to wade through if all the CARMA ambassador's log statements get mixed together, worth considering if we can add the subclass name to these log statements somehow. There's got to be a way. My java's a bit rusty so I'm not sure what this.getClass() would return here, whether that'd give the superclass or the subclass, but something along those lines has to work.

[EricChen-Lei]

I actually tested with this.getClass(), the only problem I potentially see it's maybe it too long and complicate for a log, so I use this.getClass().getSimpleName() instead, it ouputs the class name but not the package name.

[kjrush]

Yeah, I remember that method now. getSimpleName() should work great for what we're looking for. I just wasn't sure whether it'd return "CommonMessengerAmbassador" or the name of the actual subclass, but if it does actually return the subclass name then that's exactly what we'd want!

[sonarcloud]

Quality Gate failed

Failed conditions
48.05% Line Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[kjrush]

I think these changes look good to me, there might be some further cleanup that could be done w.r.t. the class names being used in logging, but I don't think that's worth holding this merge up. If you want to add the class name more broadly to the log statements as a prefix "<CLASS_NAME>: New instance registered of type ...." etc. then please do so, but I'll leave that up to you. I'll approve for now so you can merge whenever you're ready.