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

Network interface: Add status callback register #5457

Merged
merged 16 commits into from Jan 3, 2018

Conversation

@TeemuKultala
Contributor

TeemuKultala commented Nov 8, 2017

Description

This adds connection status callback registering, and connection status reporting to NetworkInterface, and implementation of these to CellularInterface, mesh and ethernet interfaces

Status

READY

Migrations

YES

In NetworkInterface new APIs:
-attach() for connection status callback registering
-get_connection_status()
-set_blocking()

In PPPCellularInterface:
-connection_status_cb() removed

Related PRs

Todos

  • Documentation needs to be updated

Deploy notes

Steps to test or reproduce

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

The destination branch: feature-status-callbacks . Is that correct?

Would @hasnainvirk review this, anyone else should be asked for review for this feature?

IN DEVELOPMENT

Let us know once this be ready .

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 8, 2017

@0xc0170 Yes, the destination branch is feature-status-callbacks.

Possible list of reviewers:
@hasnainvirk, @SeppoTakalo, @kjbracey-arm, @mikaleppanen, @mikter, @AnttiKauppila

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Nov 8, 2017

@TeemuKultala
Please add reviewers here:
image

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 8, 2017

@SeppoTakalo Note, there are currently some problems with access for reviewers (I can not assign most of the names mentioned here), we are investigating

@hasnainvirk

Looks okay to me for going into feature-status-callbacks. I would suggest taking all NetworkInterface users in confidence. This is going to effect them mildly.

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 9, 2017

@SeppoTakalo, @0xc0170 I cannot modify the Reviewers list

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Nov 9, 2017

@kjbracey-arm Please review.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Nov 13, 2017

@TeemuKultala what does "Status cb pr1" mean? Could you update your PR title?

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 15, 2017

@theotherjimmy "Status cb pr1" means "status callback pull request 1". I try to find a better name for this or for the next pull request

*
* @param status_cb The callback for status changes
*/
virtual void register_status_callback(mbed::Callback<void(ConnectionStatusType)> status_cb);

This comment has been minimized.

@0xc0170

0xc0170 Nov 16, 2017

Member

should this follow attach() for attaching a callback?

@@ -31,6 +32,11 @@ class NetworkStack;
*/
class NetworkInterface {
public:
enum ConnectionStatusType {
down, local_up, global_up, undefined

This comment has been minimized.

@0xc0170

0xc0170 Nov 16, 2017

Member

This should start uppercase Down, etc, each of them on the new line, good if with doxygen description for each status type.

*
* @return The connection status according to ConnectionStatusType
*/
virtual ConnectionStatusType get_connection_status();

This comment has been minimized.

@0xc0170

0xc0170 Nov 16, 2017

Member

why is this not connection_status_t ? same as nsapi_error_t on the line 152

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

Implementations needs to be still added to mesh and ethernet interfaces

This should be part of this patch, thus this needs some work (not yet complete) ?

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 16, 2017

@0xc0170 I have now ethernet and mesh changes available, and also fixes for other things you mentioned, should I make a new pull request for those things, or add them to this same pull request?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 16, 2017

You can add them here, feel free to push updates anytime

@0xc0170 0xc0170 added needs: work and removed needs: review labels Nov 16, 2017

@0xc0170 0xc0170 changed the title from Status cb pr1 to Network interface: Add status callback register Nov 16, 2017

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 20, 2017

@0xc0170 Ethernet and mesh changes added here now, so should the status change back to needs: review, or how to proceed?

@0xc0170 0xc0170 added needs: review and removed needs: work labels Nov 20, 2017

*
* @param status_cb The callback for status changes
*/
virtual void register_status_callback(mbed::Callback<void(connection_status_t, int)> status_cb);

This comment has been minimized.

@0xc0170

0xc0170 Nov 20, 2017

Member

Still not answered, is this regular way how to register a callback? many API in mbed has attach, shall this be also aligned?

This comment has been minimized.

@0xc0170

0xc0170 Nov 21, 2017

Member

What about how to register callback? I would like to have an alignment if possible or an argument why this is register_status_callback_name

@@ -55,6 +55,14 @@ enum nsapi_error {
NSAPI_ERROR_CONNECTION_TIMEOUT = -3017, /*!< connection timed out */
};
typedef enum connection_status {
DISCONNECTED,

This comment has been minimized.

@0xc0170

0xc0170 Nov 20, 2017

Member

this should follow the rest of enumeration values (add prefix NSAPI), plus more specification (NSAPI_CONNECTION_STATUS or at least NSAPI_STATUS). See line 55 and others

@TeemuKultala

This comment has been minimized.

Contributor

TeemuKultala commented Nov 21, 2017

@0xc0170 register_status_callback renamed to attach, enum changed too

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 21, 2017

@TeemuKultala There are lot of reviewers assigned but only two reviewed so far.
I restarted jenkins PR , if it fails , please review the results

@kjbracey-arm @hasnainvirk Review please

Update: attach now , I see. Thanks

@kjbracey-arm

kjbracey-arm requested changes Nov 23, 2017 edited

Looks like this is going to work out okay; main issue is that it doesn't seem to have picked up on the main point of the review with @AnttiKauppila and @lauri-piikivi about the callback API, which is to make the "connection status change" just one possible event on the attach, so the new state becomes a subreason of that.

This is going to conflict thoroughly with #5558 - whatever this does is going to have to be reimplemented in line with its style. That needn't worry you excessively, but it may help concentrate the mind with regard to genericity.

{
}
Callback<void(nsapi_connection_status_t, int)> EthernetInterface::_connection_status_cb = NULL;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

These things mustn't be static. Nothing stopping you having multiple Ethernet interfaces.

*
* @param status_cb The callback for status changes
*/
virtual void attach(mbed::Callback<void(nsapi_connection_status_t, int)> status_cb);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

The callback here isn't following the discussion we had in that 2017-11-08 meeting - that agreed generalising to be effectively (nsapi_event_t reason, uint32_t parameter), so for this initial event the "reason" would be "connection status change", and the parameter would be the nsapi_connection_status_t.

@@ -111,6 +123,12 @@ class EthernetInterface : public EthInterface
char _ip_address[IPADDR_STRLEN_MAX];
char _netmask[NSAPI_IPv4_SIZE];
char _gateway[NSAPI_IPv4_SIZE];
static Callback<void(nsapi_connection_status_t, int)> _connection_status_cb;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

As mentioned above - nothing should be static

@@ -599,7 +617,12 @@ nsapi_error_t mbed_lwip_bringup_2(bool dhcp, bool ppp, const char *ip, const cha
netif_set_default(&lwip_netif);
netif_set_link_callback(&lwip_netif, mbed_lwip_netif_link_irq);
netif_set_status_callback(&lwip_netif, mbed_lwip_netif_status_irq);
if (status_cb) {
netif_set_status_callback(&lwip_netif, status_cb);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

Don't you always need to call our callback to unblock the blocking connect? Oh, you've assuming the person doing the attach will call back in to our status callback.

No that doesn't make sense, and will cause complication with the stack generalisation (#5558). The callback to lwIP should always be mbed_lwip_netif_status_irq. That can call status_cb if set - you'll need to record its setting.

nsapi_error_t NetworkInterface::set_blocking(bool blocking)
{
return NSAPI_ERROR_UNSUPPORTED;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

If you're adding this, you should implement it for lwIP and Nanostack, which I don't think you are.

*
* @return The connection status according to ConnectionStatusType
*/
virtual nsapi_connection_status_t get_connection_status();

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

Should be const.

/** Set blocking status of connect()
*
* @param blocking true if connect is blocking

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

Note that the default is blocking.

@@ -263,9 +263,11 @@ PPPCellularInterface::PPPCellularInterface(FileHandle *fh, bool debug)
_fh = fh;
_debug_trace_on = debug;
_stack = DEFAULT_STACK;
_connection_status_cb = NULL;
_ppp_cb = Callback<void(nsapi_connection_status_t, int)>(this, &PPPCellularInterface::ppp_status_cb);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

I think you should be able to simplify this to callback(this, &PPPCellularInterface::ppp_status_cb). Although not sure why it's a variable at all? Just pass that straight to the bringup function.

@@ -398,7 +406,11 @@ bool PPPCellularInterface::nwk_registration(uint8_t nwk_type)
bool PPPCellularInterface::is_connected()
{
return dev_info.ppp_connection_up;
if (_connect_status == NSAPI_STATUS_DISCONNECTED) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Nov 23, 2017

Contributor

return __connect_status != NSAPI_STATUS_DISCONNECTED; perhaps?

@0xc0170 0xc0170 added the needs: work label Nov 23, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 2, 2018

Build : SUCCESS

Build number : 778
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5457/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@TeemuKultala TeemuKultala force-pushed the TeemuKultala:status_cb_PR1 branch from c498a6b to 956fddd Jan 3, 2018

@TeemuKultala TeemuKultala force-pushed the TeemuKultala:status_cb_PR1 branch from 956fddd to 2fcbf06 Jan 3, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 3, 2018

Build : SUCCESS

Build number : 784
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5457/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 469d74f into ARMmbed:feature-status-callbacks Jan 3, 2018

18 checks passed

AWS-CI uVisor Build & Test Verification build successful.
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@0xc0170 0xc0170 removed the needs: work label Jan 3, 2018

@jarlamsa jarlamsa referenced this pull request Feb 7, 2018

Merged

Status callbacks #6032

jarlamsa added a commit to jarlamsa/mbed-os that referenced this pull request Feb 9, 2018

NSAPI status callback changes
This is the original content of feature-status-callbacks, reviewed in
ARMmbed#5457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment