Conversation
socom is a high level SOME/IP abstraction which treats payloads as binary data. It has a plugin mechanism to support different underlying transport implementations (IPC, SOME/IP).
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
lurtz
left a comment
There was a problem hiding this comment.
These are the changes proposed for zero copy
|
The created documentation from the pull request is available at: docu-html |
NEOatNHNG
left a comment
There was a problem hiding this comment.
First glimpse before the weekend.
|
|
||
| [[nodiscard]] | ||
| virtual Result<std::unique_ptr<Writable_payload>> allocate_method_payload( | ||
| Method_id /* method_id */) noexcept { |
There was a problem hiding this comment.
As we already discussed in the meeting I think it would make sense to have the events/methods as their individual interface class so that you can call Allocate()/Call()/Send() on the element without always having to specify the IDs.
There was a problem hiding this comment.
I will try to create something and use inspiration from mw::com.
There was a problem hiding this comment.
There was a problem hiding this comment.
Have a look at the state of a34dc64
That is the final commit of the API refactoring. I think the API is cleaner and better structured and still offers the same functionality. But compared to the previous state both Client_connector and Server_connector need more member variables. Maybe I can bring this down with some refactoring.
Also the get_events() and get_methods() functions create a new std::vector. I do not know if that is good.
| /// \param client_id ID of the event. | ||
| /// \param mode Mode of the event. | ||
| /// \return Void in case of successful operation, otherwise an error. | ||
| virtual Result<Blank> subscribe_event(Event_id client_id, Event_mode mode) const noexcept = 0; |
There was a problem hiding this comment.
On the client side does it make sense to specify the mode? Isn't the mode determined by the server side?
There was a problem hiding this comment.
The mode is supposed to distinguish Events and Fields.Event_mode::update is normal events, where you might not have an initial value and Event_mode::update_and_initial_value is for fields, where you should immediately get you event update callback called after subscription.
There was a problem hiding this comment.
Yes, understood. But does it make sense to have that on the client side? Because normally the server needs to indicate whether the event has an initial value, right?
There was a problem hiding this comment.
What do you think about moving it to the configuration?
There you can only specify the number of events. Instead of a number, this could be a list of Event_mode::update and Event_mode::update_and_initial_value.
The structure is not required by the Client_connector and it can work with a subset of this struct.
| namespace score::socom { | ||
|
|
||
| /// \brief Error conditions when using Client_connector. | ||
| enum class Error : std::uint8_t { |
There was a problem hiding this comment.
Should that be adjusted to the general ErrorCode pattern?
There was a problem hiding this comment.
I implemented a not completely cleaned up version (to keep the diff short) of this: fc0376c
I am hot 100% happy with the result. Inspired from Rust I wanted to make it very clear that all possible values specified in an error enum are actually possibly to return. This is the reason why there are specialized enums with subsets or other values. Otherwise I would have had only a single error enum for socom.
| namespace score::socom { | ||
|
|
||
| /// \brief Alias for an event ID. | ||
| using Event_id = std::uint16_t; |
There was a problem hiding this comment.
If we keep the concrete IDs out of the definitions we would have a generic service API that we could maybe also use for other network protocol stacks (e.g. DDS?)
If we use something like CreateRemoteEvent(std::string_view event_name) to create a specific object then ID to be used could be fetched from the config.
The upper layer code still has to be changed to serialize the payload differently but at least we could re-use the same API.
There was a problem hiding this comment.
The Event_id is not the SOME/IP Event id. It is a consecutive integer range starting from 0. Thus the first SOME/IP event of a service interface is mapped to Event_id{0} and so on. This implies all parties agree on the ordering of events and if they do not, we might need something like you proposed.
There was a problem hiding this comment.
Hmm, then I think we need a name because having an index seems too error prone in my experience.
There was a problem hiding this comment.
We have to find a design, where either string comparisons are cheap or we do as less as possible.
Also keep in mind that my intention was not to read configuration files within the socom code. It is the code using socom, which is supposed to do that.
My first idea would be that each side passes a list of strings and socom::Runtime creates a mapping. However this way we will end up with n^2 string comparisons when connecting client and server connector per service. That might cause too much overhead at system startup.
There was a problem hiding this comment.
I see what you mean. If we really go the direction of sending the configuration over to the other end when connecting then we probably won't have a mismatch. Then we can also go back to the old implementation of dispatching everything by the ID. That way we would have to do the dispatch to the different service components in the RemoteServiceInstance, but I guess that is OK and would also save us some callback objects.
This is the standard way to return error codes in the SCORE codebase.
NEOatNHNG
left a comment
There was a problem hiding this comment.
Reviewed the design and most of the API which seems fine so far. What I have not looked into yet is the implementation. Does it make sense to have a look into the implementation yet or do we want to simplify first?
| Available --> Not_available: Client_connector::\nsubscribe_event() | ||
| Available --> Event_subscribed: Client_connector::\nsubscribe_event() |
There was a problem hiding this comment.
Under which condition do we switch to Event_subscribed or Not_available? When subscribe_event() returns an error?
There was a problem hiding this comment.
This looks wrong to me. When we have an Enabled_server_connector the only error, which can happen on the Client_connector::subscribe_event() side is using an out of range id. However this will not disconnect both and instead just return an error.
| state Event_subscribed | ||
| state Event_subscribed_acknowledged |
There was a problem hiding this comment.
This is a bit confusing. Maybe rename like this?
| state Event_subscribed | |
| state Event_subscribed_acknowledged | |
| state Event_subscription_pending | |
| state Event_subscribed |
There was a problem hiding this comment.
The whole sequence diagram looks like it needs improvement. I will do my best
There was a problem hiding this comment.
I think the confusion comes from the fact that events can be received in both states. The acknowledged state is optional.
There was a problem hiding this comment.
Hmm, so the receival of the ACK switches to the Event_subscribed_acknowledged state but has no further effect? Should we then remove it?
On the protocol level the ACK is needed so that the subscription request can be resent in case it is lost on the remote end, but on the local machine we probably don't need the application to know about this because it shouldn't need to resend the subscription request, correct?
If we don't need it I think we should remove it to keep it simple.
There was a problem hiding this comment.
I guess mw::com does not have similar API either. I remove it
| Event_subscribed --> Not_available: Client_connector::Callbacks::\non_service_state_change\n(!Service_state::available) | ||
|
|
||
| Event_subscribed_acknowledged --> Available: Client_connector::\nunsubscribe_event() | ||
| Event_subscribed_acknowledged --> Event_subscribed: Client_connector::Callbacks::\non_event_subscription_status_change\n(Event_state::unsubscribed) |
There was a problem hiding this comment.
How to get from Event_subscribed to Event_subscribed_acknowledged again if we got there via this transition? Is the subscription retried automatically (i.e. responsibility of the server connector)?
There was a problem hiding this comment.
IMHO two state machines are mixed here. The one acknowledging event subscriptions is not required to be used: https://github.com/elektrobit-contrib/eclipse-score_inc_someip_gateway/blob/a34dc64af9901e8e883c730fc8cdcadf8bfe921d/src/socom/include/score/socom/server_connector.hpp#L198-L206
Thus event transmission works independently of calling Enabled_server_connector::set_subscription_state(). Which also means no Client_connector will be connected or disconnected when this function is called.
As far as I can see the Client_connector will get unsubscribed from all events offered by an Enabled_server_connector, when the Enabled_server_connector gets destroyed or converted to Disabled_server_connector. In both cases events are not resubscribed automatically once we get the Enabled_server_connector back and the user of the Client_connector has to do it explicitly.
There was a problem hiding this comment.
Forget my previous answer.
How to get from Event_subscribed to Event_subscribed_acknowledged again if we got there via this transition?
This state transition is toggled via Enabled_server_connector::set_subscription_state()
switch to Not_available happens when Callback gets called
IMHO we should settle on the API first. We can peek into the implementation to make sure that the API is not expensive. |
24fea57 to
5d8e581
Compare
The socom tests have a very good coverage and also do stress tests to detect race conditions in the implementation. They are written as black box tests and make no assumption of the inner architecture of socom.
socom is a high level SOME/IP abstraction which treats payloads as binary data. It has a plugin mechanism to support different underlying transport implementations (IPC, SOME/IP).
The current code still supports 1:n connections for events. We assume that we only need 1:1 and can simplify the code and introduce zero copy. However before doing that I would more thoroughly check our use cases.
Potentials for simplifications
Runtime::subscribe_find_service()functionsRuntime::subscribe_find_service()TODO