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

Clarify asynchronous NetworkInterface::connect() documentation #8836

Merged
merged 1 commit into from
Nov 25, 2018

Conversation

SeppoTakalo
Copy link
Contributor

Description

Clarify asynchronous NetworkInterface::connect() documentation.

In non-blocking case, the return values differ from what Socket::connect() would return in the equivalent situation. Therefore the documentation needs clarification.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[x] Docs update
[ ] Test update
[ ] Breaking change

@SeppoTakalo
Copy link
Contributor Author

@kjbracey-arm @blind-owl Please review.

* asynchronous (non-blocking) mode by calling NetworkInterface::set_blocking(false).
*
* @return NSAPI_ERROR_OK on success, or if asynchronous operation started.
@ @return negative error code on failure.
*/
virtual nsapi_error_t disconnect() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add Teppo as participant to this review

  1. we (cellular subsystem nor ConnectionHelper) module do not have requirement for asynchronous disconnect()
  2. within this context set_blocking API documenttation is broken as scoped only for connect() use case
    Is my interpretation below of the API behaviour correct?
  3. issue disconnect() when asynchronous (non-blocking) mode set
    a) connection state != disconnected
    NSAPI_ERROR_OK on success, or if asynchronous operation started NSAPI_STATUS_DISCONNECTED event will be send upon completion of the operation
    a) connection state == disconnected
    NSAPI_ERROR_OK on success, @note no completion event will be send in case when asynchronous (non-blocking) mode set

Copy link
Contributor

@kjbracey kjbracey Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Events are always sent on state changes if you've registered for them, regardless of the set_blocking setting of the calls. (At least that was the design intent, if not clear from documentation, documentation needs update).

Copy link
Contributor

@blind-owl blind-owl Nov 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this means in the case of 3b) above, when no action is taken by the implementation it should schedule dispatching an NSAPI_STATUS_DISCONNECTED event anyway?

Above you mentioned a state change but within this context no state change is done so based on the logic you commented no NSAPI_STATUS_DISCONNECTED event should be dispatched as no state change was done.

Agree completely with you that all state transit events should be dispatched but as said above this not the scope of this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If state doesn't change, there's no state change callback. So if the disconnect call doesn't do anything (cos you're already disconnected), no callback. Callbacks are the consequence of state changes, which may or may not be caused by connect/disconnect requests, or environment changes.

If already disconnected when a disconnect call is made, I'd expect it to return NSAPI_ERROR_NO_CONNECTION - add that for symmetry with the connect-when-connected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most current implementations of connect() are written as "put state machine into connecting mode", then "if blocking wait for up state.". The state machine sends callbacks regardless. The time of the wait is interface-dependent.

If blocking, return code indicates whether connection was achieved by time of return. If non-blocking, Nanostack and LWiP just say "OK" indicating we're now in "connecting" state and hopefully on our way to a future "up" callback. Possibly that should have been "in progress", but this change formalises the OK.

Maybe this needs to clarify that difference between blocking and non-blocking returns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for symmetry, disconnect should really be the same, but at present all calls are blocking (although in most cases they work fast enough to not really make a difference).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no more input to provide => you could just update the API aligning to the comments so we can take a new look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following scenario should also be supported by the API, which I believe currently is not

modet set: non-blocking
OP sequence:
OP1: disconnect() => return NSAPI_ERROR_OK (OP accepted for execution)
OP2: connect() => return ?? (OP NOT accepted for execution as OP1 allready running)

@note: sequence could also be vice versa

Checking through the enum variables only one, which makes sense, is NSAPI_ERROR_IN_PROGRESS (in place off ??)

Comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When nothing else matches, use NSAPI_ERROR_DEVICE_ERROR

@blind-owl
Copy link
Contributor

@jarvte please review

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 24, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 25, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts
Build logs

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

Successfully merging this pull request may close these issues.

6 participants