-
Notifications
You must be signed in to change notification settings - Fork 5
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
Initial implementation of up-transport-vsomeip-rust #2
Initial implementation of up-transport-vsomeip-rust #2
Conversation
TODO: * Ensure we comply with the 1.5.8 spec for validation * Iron out remaining TODOs / questions around uProtocol <-> vsomeip * Add more unit tests * Add more integration tests * Add CI to be triggered on PR
* Added documentation on how to build, run * Modified build process so that we can specify where the vsomeip shared libraries are located
d83b3b1
to
52bb0f1
Compare
52bb0f1
to
43ce45b
Compare
…r purposes of getting all point-to-point messages
…g get_data_safe(), as data_ptr null means vsomeip is saying there's no data in the payload Added logging around setting & getting payload of vsomeip message
src/determinations.rs
Outdated
(most_significant_bits, least_significant_bits) | ||
} | ||
|
||
pub(crate) fn split_u32_to_u8(value: u32) -> (u8, u8, u8, u8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use https://doc.rust-lang.org/std/primitive.u32.html#method.to_le_bytes
instead of manual shifting and masking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments/clarifying questions
#[allow(dead_code)] | ||
config_path: Option<PathBuf>, | ||
// if this is not None, indicates that we are in a dedicated point-to-point mode | ||
point_to_point_listener: RwLock<Option<Arc<dyn UListener>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why a RwLock is needed and why you chose to wrap the Option rather than Option<RwLock<...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure -- I used a RwLock here because I needed a means to ensure thread-safety of the Option<> since it's used within different async functions.
Ok(()) | ||
} | ||
|
||
async fn register_point_to_point_listener( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general question I have here is whether there is only one point to point listener allowed for one utransport instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- this is to support the uStreamer use case. If we're in that point-to-point listener mode then we will make use of the applications configured in the vsomeip config JSON file.
src/lib.rs
Outdated
UUri, | ||
Option<UUri>, | ||
RegistrationType, | ||
oneshot::Sender<Result<(), UStatus>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain what the oneshot does here? Based on my understanding of the code, its a sender that is used by the event loop to return the execution status of the TransportCommand, and since a TransportCommand only gets executed once a oneshot makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, you are correct.
src/transport.rs
Outdated
pub(crate) static ref LISTENER_ID_MAP: ListenerIdMap = RwLock::new(HashMap::new()); | ||
} | ||
|
||
generate_message_handler_extern_c_fns!(10000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this macro creates the 'FREE_LISTENER_IDS' lazy static map, which is used to cap the number of listeners for the client and ensure that each listener has a unique id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the proc macro crate's definition of generate_message_handler_extern_c_fns. Long story short is that since an extern "C" fn is required by the vsomeip library to register a message handler function, we must pre-create all these extern "C" fns.
Internal to each extern "C" fn they call call_shared_extern_fn function which will then use the map to pick out the correct UListener to trigger.
src/transport.rs
Outdated
{ | ||
let mut listener_client_id_mapping = LISTENER_CLIENT_ID_MAPPING.write().await; | ||
listener_client_id_mapping.remove(&listener_id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these writes all happen in the same block since they are all related to the same unregister request? I would be a bit worried that there is a scenario where a mismatch could happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, okay, I'll think on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to group these together based on your feedback.
22fd3e6
to
aafb3c5
Compare
aafb3c5
to
0b42c81
Compare
src/lib.rs
Outdated
const UP_CLIENT_VSOMEIP_FN_TAG_INITIALIZE_NEW_APP_INTERNAL: &str = "initialize_new_app_internal"; | ||
const UP_CLIENT_VSOMEIP_FN_TAG_START_APP: &str = "start_app"; | ||
|
||
// TODO: Revisit what authority to attach, if any, to the remote mE. For now, just assign "me_authority" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenhartley -- SOME/IP doesn't have a concept of an authority, so here I've chosen to attach this.
Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authority in the case of SOME/IP devices will refer the IP addresses of the devices that the mechatronics services run on. We are either going to use the static IP address or the hostnames (TBD) but we need to fetch that information (it is sen din the offer_service() AFAIK).
src/lib.rs
Outdated
UP_CLIENT_VSOMEIP_FN_TAG_REGISTER_LISTENER_INTERNAL, | ||
); | ||
let (_, service_id) = split_u32_to_u16(source_filter.ue_id); | ||
// let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to understand what this is used for. we are not telling SOME/Ip that we want to "register for all published events" but we need to subscribe to only the events/messages that local (high compute) entities want to subscribe to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make a helper function that returns (instance_id, service_id)
from ue_id
as specified in: https://github.com/eclipse-uprotocol/up-spec/blob/main/up-l1/someip.adoc#uuris
src/lib.rs
Outdated
}; | ||
|
||
let (_, service_id) = split_u32_to_u16(sink_filter.ue_id); | ||
let instance_id = 1; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instance ID will eventually be defined in some uService artifact file and then fed intot he static JSON that SOME/IP needs to function but for now we assume 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use high 16 bits of ue_id
as instance. If they are 0x0000
, assume instance_id=1
src/lib.rs
Outdated
); | ||
|
||
let (_, service_id) = split_u32_to_u16(source_filter.ue_id); | ||
let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to accept responses from anyone (any uE ID and andy instance id).
@int0x27 do you know anymore here?
src/lib.rs
Outdated
UP_CLIENT_VSOMEIP_FN_TAG_REGISTER_LISTENER_INTERNAL, | ||
); | ||
let (_, service_id) = split_u32_to_u16(source_filter.ue_id); | ||
let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above AFAIK
src/lib.rs
Outdated
}; | ||
|
||
let (_, service_id) = split_u32_to_u16(sink_filter.ue_id); | ||
let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
src/lib.rs
Outdated
}; | ||
|
||
let (_, service_id) = split_u32_to_u16(sink_filter.ue_id); | ||
let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should this be set to? Wondering if you know @int0x27 / @stevenhartley
src/lib.rs
Outdated
); | ||
|
||
let payload = get_message_payload(&mut vsomeip_msg).get_shared_ptr(); | ||
// TODO: Note that we cannot set the interface_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't seem like I can set the interface_version / ue_version_major here. Wondering if y'all know if this is okay @int0x27 / @stevenhartley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the major version
is reused from application::offer_service in the router.
make_message_wrapper(get_pinned_runtime(runtime_wrapper).create_notification(true)); | ||
let (_instance_id, service_id) = split_u32_to_u16(source.ue_id); | ||
get_pinned_message_base(&vsomeip_msg).set_service(service_id); | ||
let instance_id = 1; // TODO: Setting to 1 manually for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC from reading the SOME/IP spec, I do need to respect the upper four bytes of the ue_id and assign the instance_id appropriately according to them, right? Tagging @stevenhartley & @int0x27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, make a helper function to parse ue_id
as (instance_id, service_id)
. Make sure instance_id 0x0000 is replaced with 0x0001
service_id | ||
); | ||
get_pinned_message_base(&vsomeip_msg).set_service(service_id); | ||
let instance_id = 1; // TODO: Setting to 1 manually for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC from reading the SOME/IP spec, I do need to respect the upper four bytes of the ue_id and assign the instance_id appropriately according to them, right? Tagging @stevenhartley & @int0x27
src/message_conversions.rs
Outdated
}; | ||
|
||
let source = UUri { | ||
authority_name: ME_AUTHORITY.to_string(), // TODO: Should we set this to anything specific? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as here
let sink = UUri { | ||
authority_name, | ||
ue_id: client_id as u32, | ||
ue_version_major: 1, // TODO: I don't see a way to get this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a way to obtain the ue_version_major for the sink for a SOME/IP / vsomeip MT_ERROR, so I punted and set to 1 for now.
Any thoughts @stevenhartley / @int0x27?
src/message_conversions.rs
Outdated
}; | ||
|
||
let source = UUri { | ||
authority_name: ME_AUTHORITY.to_string(), // TODO: Should we set this to anything specific? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as here
…ation based on its name
let comp_listener = ComparableListener::new(listener.clone()); | ||
|
||
let src = any_uuri(); | ||
// TODO: How to explicitly handle instance_id? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we cannot set the instance_id within the vsomeip config file, how do we handle this? For now I'm setting the upper four bytes to zero.
Tagging @stevenhartley / @int0x27
…rmed about subscription changes.
src/lib.rs
Outdated
application_wrapper, | ||
service_id, | ||
instance_id, | ||
event_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using dedicated variable eventgroup_id
so it is clear what you are referring to, as eventgroup is usually different than event in someip
src/lib.rs
Outdated
instance_id, | ||
event_id, | ||
ANY_MAJOR, | ||
event_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: specifying optional event_id
here only filters incoming someip events from the eventgroup and invokes the callback only for event_id. Not an issue if you have 1:1 mapping.
src/lib.rs
Outdated
get_pinned_application(application_wrapper).offer_service( | ||
service_id, | ||
instance_id, | ||
ANY_MAJOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must specify the major / minor version when offering the service. They are optional in vsomeip, but default values are (0, 0)
Consider using uuri ue_version_major
src/lib.rs
Outdated
{ | ||
let requested_services = REQUESTED_SERVICES.write().await; | ||
if !requested_services.contains(&(service_id, instance_id, method_id)) { | ||
get_pinned_application(application_wrapper).request_service( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to call request_service()
multiple times for the same service if it has multiple methods
…eWrapper with a payload attached and then send, the payload appears to become corrupted. For now we send within the same context as we attach the payload.
aff8068
to
cc9e998
Compare
Hi folks!
Here's the initial implementation of a Rust up-transport wrapping vsomeip.
I'm trying to get something out the door to be able to get looked at, but note that it's still in draft status. There's still a fair number of TODOs scattered about. 😅
TODO: