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

Support handling multiple services with the same service and instance IDs but different major version. #829

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KareemLMR
Copy link

@KareemLMR KareemLMR commented Dec 20, 2024

This is an implementation for PRS_SOMEIPSD_00512 and PRS_SOMEIPSD_00806 as I have noticed from https://github.com/COVESA/vsomeip/issues/797 and https://github.com/COVESA/vsomeip/issues/809
That the current version of vsomeip only supports handling services with different instances, whereas according to SOME/IP Specification Document we should handle offering and requesting services with the same instance ID but different major version. It is clear in the 2 Requirements IDs I attached and even in the contradiction sentence that services with only minor versions different shouldn't be offered: "Note: Configuring more than one service instance on one Network Node that differs
only in the Minor Version is not allowed since they can not be distinguished in Eventgroup entries."

I have tried the current implementation and it was true that it doesn't handle these requirements and it is even clear that all data structures in the source code that should distinguish between services has only service ID and instance ID as keys while it should include the major version also.

And to explain what is the change in the PR in brief, I tried first the straight forward solution which is appending the major version as a key in all the maps and data structures of the source code but noticed the impact of that change would be severe. Nearly all of the code structure would be completely changed which might affect the performance and introduce many bugs besides making the code more complex and less readable since in every search loop for example we will have to nest other loops. Also from logical sense each instance is mapped to IP and Port (endpoint) like in the functions find_instance() then it would be more straightforward to make a new coupled type of instance and major version to be mapped to every endpoint so I noticed instance and major should be closely coupled. There is no point of handling a case where instance ID is the same while major is different. It should be a completely new service if so.

I also tried to split the change into parts and create a PR for every part but unfortunately that too was not possible as changing one part for example 1 map of the offered services would make the rest of the code behaves differently.

So I decided to use what I said before as making the instance and major closely coupled, either by defining a new structure calling it unique_version_t for example and pass it as a key to all maps instead of the instance_t, but this would not keep backward compatibility with the current API and will require complex overloading operators for every data structure and I am not sure it would be possible for the rest of the code.

So I went for the last solution of defining a new primitive type unique_version_t which is simply a uint32_t variable to concatenate bits of major version as the MSBs and instance as the LSBs and pass it as a key instead of the instance_t in every data structure of the code. That would completely keep the backward compatibility as if the user didn't use the inline converting function as: vsomeip_v3::unique_version_t unique = vsomeip_v3::get_unique_version(SAMPLE_INSTANCE_ID, 10);
The change would be 100% impact less as if no change was made and I tested that in fact.

By this change I believe we can keep the structure and architecture of the code as it is and make the least impact of that change and I think the only drawback of that change is that we defined a new type that doesn't exist in the SOME/IP Requirements but I believe we can simply add a comment describing the change.

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.

1 participant