Skip to content

Clarify asynchronous NetworkInterface::connect() documentation #8836

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 25, 2018
Merged

Clarify asynchronous NetworkInterface::connect() documentation #8836

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions features/netsocket/NetworkInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,27 @@ class NetworkInterface: public DNS {

/** Start the interface.
*
* @return NSAPI_ERROR_OK on success, negative error code on failure.
* This blocks until connection is established, but asynchronous operation can be enabled
* by calling NetworkInterface::set_blocking(false).
*
* In asynchronous mode this starts the connection sequence and returns immediately.
* Status of the connection can then checked from NetworkInterface::get_connection_status()
* or from status callbacks.
*
* @return NSAPI_ERROR_OK on success, or if asynchronous operation started.
* @return NSAPI_ERROR_ALREADY if asynchronous connect operation already ongoing.
* @return NSAPI_ERROR_IS_CONNECTED if interface is already connected.
* @return negative error code on failure.
*/
virtual nsapi_error_t connect() = 0;

/** Stop the interface.
*
* @return NSAPI_ERROR_OK on success, negative error code on failure.
* This blocks until interface is disconnected, unless interface is set to
* 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 •
edited
Loading

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 •
edited
Loading

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


Expand Down