Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collective Perception Service - Core #251

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

sodevel
Copy link
Contributor

@sodevel sodevel commented Jun 28, 2022

These are the core Artery changes required for #249.

The way the patching of the CP message format is done is not optimal, it results in a dirty source tree. This wasn't done by me because back in the days i had no idea about that CMake stuff, this is a bit different now. It would be better to copy the source files that require patching into the binary tree and patch them there, but currently this doesn't work because the files use quoted includes.

If i understand the CMake files of Vanetza right, they contain the code to actually compile the C++ files that are included in the repository from the ASN1 files and perform some intense post-processing. This post-processing also changes the angle includes into quoted includes. If the angle includes would be kept, putting the binary tree before the source tree in the include path would be sufficient to pick up the patched header file instead of using the original one (selecting the correct source file is easily done by CMake code).

m-wegner and others added 24 commits June 6, 2022 21:05
EnvmodPrinter did only print the Artery-internal station ID of
objects which it not persistent. Now it also prints the external
(TraCI) ID simplifying debugging of radar systems etc.
Just overriding the staged initialize method is not enough, the returned
number of init stages must also be adjusted to the number of required
stages. Additionally update the code to allow derived classes to use
single-stage initialization easily, for these classes it is enough to
just override the single-stage initialization method, it is not required
anymore to overrided the multi-stage initialization method.
`StationID` as type for the `objectID` in `PerceivedObject`
Try to match the recent code style of the project.
Add some additional settings that match the default settings
for semantic reasons.
Such module requires exactly one Simple Module Submodule
that implements the ItsG5BaseService C++ interface.
Allow longer lines and two consecutive empty lines.
This prevents a unnecessary type conversion.
Allow short blocks on a single line.
Return the internal map for entries like the LocalEnvironmentModel does
to be able to access all stores CAM's easily. Replaces the method to
return a CAM of a specific station.
@sodevel sodevel changed the title Release/cpservice core Collective Perception Service - Core Jun 28, 2022
@riebl
Copy link
Owner

riebl commented Jul 2, 2022

Thanks @sodevel for providing this branch for discussion. I am fine with most commits.

  1. Instead of hard-coding the station id assignment, we may realize a configurable mechanism supporting
  • random number (as done upstream)
  • node index (as found in your branch)
  • OMNeT++ host module id?
    What do you think?
  1. I don't quite understand the need for the ItsG5BasService multi-stage initialization. Can you provide some additional information?
  2. I am still not a fan of "hot-patching" the source code generated from CPM ASN.1. I would prefer a dedicated "CPM_alt" including "CollectivePerceptionMessage_alt" -> "CpmParameters_alt" -> "PerceivedObejctContainer_alt" -> "PerceivedObject_alt" (replace "_alt" by a reasonable suffix name). For example, we could write a CPM_alt.asn1 defining only those modified CPM types and bundle the generated in Artery. I admit this is a little bit more effort but I expect this process to be less surprising, especially for less experienced users.

@sodevel
Copy link
Contributor Author

sodevel commented Jul 6, 2022

  1. This sounds like a good idea. It's been a while i worked with OMNeT++, how should that be done? In plain C++ i would use a simple Enum together with a switch block or a function object.
  2. I don't recall exactly anymore, but in our code base there was the need to switch one of the service base classes to multi stage initialization which required to change every service to be aware of that. With that change this was not necessary anymore and you could follow the OMNeT++ policy to override only one of the initialization methods and call only their direct parent implementation.
  3. Wouldn't that require to duplicate most / much of the contents in the its folder of Vanetza? Is there a use case that requires two different CPM types to be available at the same time? I don't know if it is confusing to use different names than the ones used by the spec. Sadly, the generated code is plain C so we can't use the same names but placed into different namespaces that e.g. encode the version of the spec and patch-marker or something like that.

@riebl
Copy link
Owner

riebl commented Aug 2, 2022

Hi @sodevel, please have a look at my cpservice_core branch I have just pushed. What do you think of it?

I have implemented 1) by Identity::deriveStationId which is called by several middleware flavours, and also CarIdentityRegistrant.
Please note that the upstream CollectivePerceptionMockService already uses multi-stage initialization. No need for any changes at ItsG5BaseService for this purpose. Hence, I have omitted 2).
Modified CPM structures 3) will be part of a separate commit. I plan to merge a solution along with the modified service branch (your other PR).

I have also removed the .clang-format because it did not like Artery's existing code style at all. Maybe I am doing something wrong?

@sodevel
Copy link
Contributor Author

sodevel commented Aug 3, 2022

The branch is looking good to me, thanks for the additions.

About the clang-format specification, i tried to replicate the current code style. However, there is not a unified style used accross the whole code base, i tried to use the style that seemed current to me, i changed some small aspects because of personal preference, some results are because of limitations of clang-format itself. I did not use automatic tooling to apply the formatting, i only used the Format Document feature of VS Code regulary on the files i created. The format is not intended to be used for anything inside extern, i placed the file into the root directory only to easily apply it to scenarios and src.

Can you be more specific about what clang-format changes what it shouldn't? For testing, i formatted a bunch of files and these are my observations.

Intended changes:

  1. Tabs get converted to space. I am a space-guy, i had to do this 😄
  2. Include groups are separated by newline. This looks more clear to me than one huge include block.
  3. Some headers indent the access modifier by one level and the methods below by another level. This is kind of wasting space, i removed one level of indentation.

More or less unwanted changes:

  1. Single line loop bodies get moved in the loop header line. This one slipped through, there is a setting to prevent this, i can fix that.
  2. Multiline comments get aligned at the star. This is default behavior and is also used by Java, so i kept it.
  3. Inline comments are separated by two spaces. Again, this is default behavior, i didn't change it.
  4. The first line after many (all?) macros gets wrongfully indented by one level. This is a formatting error and the only thing i manually corrected in my files. This seems to get worse if a namespace declaration follows the macro. There are clang-format settings for macros, i could take a look at these and try to fix that.
  5. Method parameter lines and constructor initializer list lines get merged or split. This happens due to the pretty high line length limit and binpacking, there are plenty of penalty settings to tune this behavior, i didn't spend time on this.
  6. Clang-format does a pretty poor job at splitting long lines. The very latest version implements some parts of that at least 3 years old PR lingering on their issue tracker to improve that, but is this done only in certain contexts and isn't uniform. I increased the maximum line length as much as possible to reduce the need to split lines, currently we have to live with what clang-format produces.

If you are concerned about to mess up git blame output, you can add a configuration file to ignore formatting-only commits.

Let me know what i can clean up of my other PR before you merge it.

@riebl
Copy link
Owner

riebl commented Aug 15, 2022

Can you please move the clang-format stuff to a separate branch where we can further discuss the formatting-specific issues? In general, I am fine with clang-format, and I see the value of having a suitable .clang-format in the repository. However, code formatting is unrelated to Collective Perception. I much appreciate your effort!

@sodevel sodevel mentioned this pull request Sep 17, 2022
@mfyuce
Copy link

mfyuce commented Oct 15, 2022

#260

Format related PR seems to be merged.

Any update on this PR?

@riebl
Copy link
Owner

riebl commented Oct 18, 2022

I have just merged cpservice_core into master. Are you referring to any specific further parts of this PR @mfyuce?

@mfyuce
Copy link

mfyuce commented Oct 18, 2022

Thank you so much @riebl, really appreciated and want to say from my heart that really good work. I will check out.

Currently researching ways to integrate CPM messages into the VereMI dynamic dataset and hopefully open source it, if you kindly willing to accept.

Any help is appreciated.

@sodevel
Copy link
Contributor Author

sodevel commented Oct 18, 2022

This PR does not contain anything CPM related except the patching of the CPM message format which was not merged. All CPM related functionality is part of #252, but for that one to work, an alternate solution to use "extended" CPMs must be implemented first.

@mfyuce
Copy link

mfyuce commented Oct 19, 2022

Thank you @sodevel, i will look into it.

Recently read a paper implying they have integrated CPM with artery, but no source code published.

@inproceedings{tsukada2022misbehavior,
  title={Misbehavior Detection Using Collective Perception under Privacy Considerations},
  author={Tsukada, Manabu and Arii, Shimpei and Ochiai, Hideya and Esaki, Hiroshi},
  booktitle={2022 IEEE 19th Annual Consumer Communications \& Networking Conference (CCNC)},
  pages={808--814},
  year={2022},
  organization={IEEE}
}

I have contacted the authors, will inform about their response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants