Cellular: Preventing Socket ID assignment until actual socket creation at the modem #10431
Conversation
Local modem ip stacks vary in their implementations and the way of working. Some of the modems may not open a socket until an IP context is assigned. That's why we came up with a container that stores addresses of any CellularSocket instances created on-demand by the application. When the application requests opening a socket we store allocate and store the premitive in the container however actual socket creation at the modem may happen at a later stage, e.g., a call to send_to() may result in actual opening of a socket. That's why we must not assign socket ids in the CellularSocket object during construction. It must happen when actual socket is opened and is alive. Another implication of the previous model is that we may have multiple sockets created in our container but the actual socket ids are not assigned yet, so we cannot directly map the socket id to the container indices which has been happening previously. To solve this issue we have promoted the AT_CellularStac::find_socket_index(...) method to be a protected method rather than being private so that the children can use the method to determine if the given index in the container corrsponds to the assigned socket id or not. We have given up on the socket->created flag and the whole decision making to actually open a socket on the modem happens on the basis of a valid socket being assigned or not.
Socket ids will be assigned in the create_socket_impl(...) method. This is the point where an actual socket creation at the modem takes place.
Changes introduced to accomodate the socket id assignment upon actual creation of the socket at the modem.
Changes to accomodate socket id assignment upon actual creation of the socket at the modem.
This modem is a special case. It uses a given socket ID value rather than providing one. A naive solution here would be to directly map the index of a CellularSocket object in the CellularSocket container. But considering the case where there are multiple sockets being opened (some sockets being already created at the modem and some yet not created), direct mapping to indices will not work. As it can happen that the CellularSocket object is allocated but the socket id is not assigned yet as it is not actually created on the modem. In such a case, we check the container and assign the socket id from the pool if an empty slot was found.
Changes to accomodate socket id assigment upon actual creation of the socket at the modem.
hasnainvirk
force-pushed
the
sock_id_debacle
branch
from
7df8220 to
204e993
Compare
ciarmcom
requested review from
AnttiKauppila,
AriParkkila,
blind-owl,
jarvte,
kjbracey,
mirelachirica and
a team
|
@hasnainvirk, thank you for your changes. |
hasnainvirk
changed the title
There was a problem hiding this comment.
Looks fine to me.
(Probably too late, but looking at the existing code, I'd be strongly inclined to bin the _socket array and just have a linked list of CellularSocket objects - if you're using dynamic allocation anyway, then what does having a fixed-size array of pointers buy you? Just have a linked list, and make your nsapi_socket_t be the actual pointer to the CellularSocket.)
Makes sense. Will mark the suggestion as a future item. |
|
ci started |
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
|
Looks like a licence issue - restarted |
|
Restarted |
CI doesn't seem to be started. |
0xc0170
changed the title
it was restarted: PASS: 2019-04-29T10:51:03.000Z vs FAIL: 2019-04-26T00:36:37.000Z. However, the restarting process did not follow the current CI assumptions -> status not updated. |
I restarted it again |
Any more reviews for this PR? It looks ready |
|
Reminder to move this PR forward! |
|
@AnttiKauppila Code freeze for 5.13 is one week today. If you want this to go in could you please find someone to review ? |
|
@adbridge @AriParkkila Has already approved this, is that not enough? |
|
@hasnainvirk could you please rebase to remove the conflict? |
|
I would like to add this to CI, please rebase asap. |
Description
Local modem ip stacks vary in their implementations and the way of
working. Some of the modems may not open a socket until an IP context is
assigned. That's why we came up with a container that stores addresses of
any CellularSocket instances created on-demand by the application. When
the application requests opening a socket we allocate and store the
primitive in the container however actual socket creation at the modem
may happen at a later stage, e.g., a call to send_to() may result in
actual opening of a socket.
That's why we must not assign socket ids in the CellularSocket object
during construction. It must happen when actual socket is opened and is
alive.
Another implication of the previous model is that we may have multiple
sockets created in our container but the actual socket ids are not
assigned yet, so we cannot directly map the socket id to the container
indices which has been happening previously.
To solve this issue we have promoted the AT_CellularStack::find_socket_index(...) method
to be a protected method rather than being private so that the children
can use the method to determine if the given index in the container
corresponds to the assigned socket id or not.
We have given up on the socket->created flag and the whole decision
making to actually open a socket on the modem happens on the basis of a
valid socket id being assigned or not.
Pull request type
Reviewers
@AnttiKauppila @kjbracey-arm @AriParkkila @blind-owl @mirelachirica @jarvte
Release Notes