Allows multiple network status listeners #9424

SeppoTakalo · 2019-01-18T11:11:36Z

Description

Allow more than one callback to be register to NetworkInterfaces.
This introduces new APIs:

void NetworkInterface::add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);
void NetworkInterface::remove_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);

Which internally calls NetworkInterface::attach() function.

Also contains fix for: #9423

Target: Mbed OS 5.12

Release notes

NetworkInterface API is extended with two new functions regarding status callbacks. Applications now have possibility to use these new APIs to register more than one callback per network interface. New APIs are as follows:

void NetworkInterface::add_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);
void NetworkInterface::remove_event_listener(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);

Earlier NetworkInterface::attach() is still functional, and it is a porting API that each interface should provide. The new API uses internally NetworkInterface::attach() so application cannot use both APIs at the same time. Application should either be completely refactored to new API by replacing NetworkInterface::attach() calls with NetworkInterface::add_event_listener() or remain using the NetworkInterface::attach() both APIs are still supported but usage is limited to either one of these.

Pull request type

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

Reviewers

@kjbracey-arm @yogpan01 @teetak01

ciarmcom · 2019-01-18T12:00:33Z

@SeppoTakalo, thank you for your changes.
@yogpan01 @teetak01 @kjbracey-arm @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen · 2019-01-18T12:50:15Z

features/netsocket/NetworkInterface.cpp

+    }
+}
+
+void NetworkInterface::add_event_listener(void (*status_cb)(nsapi_event_t, intptr_t))


How about if you give NULL here as status_cb?

Why would you?

It is not acceptable value and leads to HardFault.

Being protective on your API and checking for NULL values can seriously hide bugs where your data types or pointers gets corrupted and are overwritten with zeros.
Then when that data is fed to here, it will be silently ignored, and device keeps running, but actually never receives callbacks.

Opposite would be that data corruption would immediately lead to HardFault when fed into APIs that try to access the value.

These Hardfault are quickly catched in the testing whereas silently ignoring carbage might lead to very hard to debug issues, as it might take long time to capture your failure and there is no knowledge anymore where it went wrong.

You should only do protective programming in external interfaces where you can expect carbage.

SeppoTakalo · 2019-01-29T13:39:34Z

This work is now complete and no blocker issues.

Can be tested and merged in.

SeppoTakalo · 2019-01-30T14:13:17Z

features/netsocket/NetworkInterface.h
=================================
Errors: 
246: unregister
272: unregister

Why unregister is not a word according to our language check?
https://en.wiktionary.org/wiki/unregister

michalpasztamobica · 2019-01-30T14:15:53Z

Why `unregister` is not a word according to our language check?
https://en.wiktionary.org/wiki/unregister

Can you try 'deregister'?

SeppoTakalo · 2019-01-30T14:16:33Z

Why `unregister` is not a word according to our language check?
https://en.wiktionary.org/wiki/unregister

Can you try 'deregister'?

No.

I'll bend the system to my will.

michalpasztamobica

I approve, but if the PR is not urgent, I think that two simple checkes could be added to unit tests:

Check that an empty listeners list causes no callbacks if the event is sent:

iface->event(NSAPI_EVENT_CONNECTION_STATUS_CHANGE, 0);
EXPECT_EQ(callback_is_called, true);

Perform the same check after the list got emptied (in case someone, intentionally or by mistake, adds a mechanism that would prevent the last listener from being removed from the list)

Allow more than one callback to be register to NetworkInterfaces. This introduces new APIs: void NetworkInterface::add_event_listener(...); void NetworkInterface::remove_event_listener(...); Which internally calls interfaces attach() functions.

->move() operator was not touching unused data fields, therefore leaving uninitialised data and failing the comparison. Fixed by initialising all fields to zero before moving.

SeppoTakalo · 2019-01-31T13:13:50Z

@michalpasztamobica Added the unittest that you proposed.

michalpasztamobica · 2019-01-31T13:15:00Z

Thank you, review reapproved after changes..

Standard new operator already calls MBED_ERROR in failure.

SeppoTakalo · 2019-02-04T08:46:42Z

@kjbracey-arm Is this error coming from ns_list.h?

SeppoTakalo · 2019-02-04T14:10:52Z

Checked with Kevin.B.
Should be fixed now.

SeppoTakalo · 2019-02-06T09:03:33Z

@0xc0170 can be tested now.

SeppoTakalo · 2019-02-06T09:39:36Z

Actually.. Don't test yet..
I can still see the failure in
https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_build-IAR/1404/

SeppoTakalo · 2019-02-06T14:13:53Z

Now should pass the IAR tests. Verified in https://mbed-os.mbedcloudtesting.com/job/mbed-os-ci_fork-test/88/

SeppoTakalo · 2019-02-06T14:14:27Z

@cmonr or @0xc0170 This can be now tested. Thanks.

adbridge · 2019-02-07T13:34:46Z

@michalpasztamobica @kjbracey-arm @VeijoPesonen @KariHaapalehto could you please re-review the latest updates from Seppo.

adbridge · 2019-02-07T13:47:11Z

CI started

0xc0170 · 2019-02-07T13:54:32Z

Requested adding release notes section (@AnotherButler to review once added)

mbed-ci · 2019-02-07T18:45:32Z

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SeppoTakalo · 2019-02-08T10:17:33Z

I can see that within the failing test job there is 1060 / SKIP / NUCLEO_F429ZI /and within those log files, it looks like this:

...
	tests-mbed_hal-rtc
	test filtered out 'tests-mbed_hal-rtc'
	tests-integration-basic
	test filtered out 'tests-integration-basic'
	tests-mbed_functional-functionpointer
	test filtered out 'tests-mbed_functional-functionpointer'
	tests-mbed_drivers-sleep_lock
	test filtered out 'tests-mbed_drivers-sleep_lock'
mbedgt: invalid test case names (specified with 'skip-test' option)
mbedgt: test name 'tests-usb_device-*' not found in 'runtime_load' (specified with --test-spec option)
	note: test case names are case sensitive
	note: see list of available test cases below
mbedgt: available tests for build 'NUCLEO_F429ZI-GCC_ARM', location 'BUILD/tests/NUCLEO_F429ZI/GCC_ARM'
mbedgt: running 0 tests for platform 'NUCLEO_F429ZI' and toolchain 'GCC_ARM'
	use 1 instance of execution threads for testing
mbedgt: all tests finished!
mbedgt: shuffle seed: 0.1498677645
mbedgt: exporting to JUNIT file 'results/report_test_greentea_NUCLEO_F429ZI_GCC_ARM.xml'...
mbedgt: exporting to HTML file 'results/report_test_greentea_NUCLEO_F429ZI_GCC_ARM.html'...
mbedgt: no platform/target matching tests were found!
mbedgt: completed in 0.13 sec
mbedgt: exited with code -10

What does this mean? It did not run anything...

alekla01 · 2019-02-08T11:35:36Z

The greentea-test was restarted and not commented here as restarted, my bad.
@SeppoTakalo it was skipped, because all the test binaries (with the same hash) had already passed previously, no reason to retest.

SeppoTakalo · 2019-02-08T13:57:29Z

@alekla01 Thanks for the clarification.

ciarmcom requested review from kjbracey, teetak01, yogpan01 and a team January 18, 2019 12:00

ciarmcom added the needs: review label Jan 18, 2019

VeijoPesonen reviewed Jan 18, 2019

View reviewed changes

KariHaapalehto approved these changes Jan 21, 2019

View reviewed changes

SeppoTakalo mentioned this pull request Jan 29, 2019

Callback object comparison not working #9423

Closed

SeppoTakalo force-pushed the ONME-4125 branch from 2384b68 to 95f311c Compare January 29, 2019 13:36

SeppoTakalo changed the title ~~WIP: Allows multiple network status listeners~~ Allows multiple network status listeners Jan 29, 2019

michalpasztamobica approved these changes Jan 30, 2019

View reviewed changes

cmonr added release-version: 5.12.0-rc1 needs: work and removed needs: review labels Jan 30, 2019

Seppo Takalo added 3 commits January 31, 2019 15:04

Allow multiple network status listeners

9c98d15

Allow more than one callback to be register to NetworkInterfaces. This introduces new APIs: void NetworkInterface::add_event_listener(...); void NetworkInterface::remove_event_listener(...); Which internally calls interfaces attach() functions.

Fix copy constructors of Callback objects

207eddc

->move() operator was not touching unused data fields, therefore leaving uninitialised data and failing the comparison. Fixed by initialising all fields to zero before moving.

Add "unregister" to valid word list

e9f5ed4

SeppoTakalo force-pushed the ONME-4125 branch from d6c6248 to e9f5ed4 Compare January 31, 2019 13:04

Add unittest to verify removal of last event listener

86019af

michalpasztamobica approved these changes Jan 31, 2019

View reviewed changes

Call "new" instead of no-throw version and the MBED_ERROR.

06aea98

Standard new operator already calls MBED_ERROR in failure.

kjbracey approved these changes Jan 31, 2019

View reviewed changes

cmonr removed the needs: CI label Feb 2, 2019

SeppoTakalo force-pushed the ONME-4125 branch from 0f5bfc0 to 26c57fc Compare February 4, 2019 13:36

SeppoTakalo mentioned this pull request Feb 4, 2019

Support C++03 compilers that cannot return properly-typed pointers. PelionIoT/nanostack-libservice#84

Merged

Fix ns_list for C++03 compilers, like IAR 7

43a53df

SeppoTakalo force-pushed the ONME-4125 branch from 26c57fc to 43a53df Compare February 6, 2019 13:26

adbridge added needs: review and removed needs: work labels Feb 7, 2019

KariHaapalehto approved these changes Feb 7, 2019

View reviewed changes

kjbracey approved these changes Feb 7, 2019

View reviewed changes

adbridge added needs: CI and removed needs: review labels Feb 7, 2019

0xc0170 requested a review from AnotherButler February 7, 2019 13:54

NirSonnenschein added ready for merge and removed needs: CI labels Feb 10, 2019

0xc0170 merged commit 10bb66a into ARMmbed:master Feb 11, 2019

0xc0170 removed the ready for merge label Feb 11, 2019

SeppoTakalo mentioned this pull request Feb 20, 2019

Various clarifications for NetworkInterface state and connection ARMmbed/mbed-os-5-docs#957

Merged

Allows multiple network status listeners #9424

Allows multiple network status listeners #9424

Uh oh!

Conversation

SeppoTakalo commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jan 18, 2019

Uh oh!

VeijoPesonen Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Jan 29, 2019

Uh oh!

SeppoTakalo commented Jan 30, 2019

Uh oh!

michalpasztamobica commented Jan 30, 2019

Uh oh!

SeppoTakalo commented Jan 30, 2019

Uh oh!

michalpasztamobica left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Jan 31, 2019

Uh oh!

michalpasztamobica commented Jan 31, 2019

Uh oh!

SeppoTakalo commented Feb 4, 2019

Uh oh!

SeppoTakalo commented Feb 4, 2019

Uh oh!

SeppoTakalo commented Feb 6, 2019

Uh oh!

SeppoTakalo commented Feb 6, 2019

Uh oh!

SeppoTakalo commented Feb 6, 2019

Uh oh!

SeppoTakalo commented Feb 6, 2019

Uh oh!

adbridge commented Feb 7, 2019

Uh oh!

adbridge commented Feb 7, 2019

Uh oh!

0xc0170 commented Feb 7, 2019

Uh oh!

mbed-ci commented Feb 7, 2019

Test run: FAILED

Uh oh!

SeppoTakalo commented Feb 8, 2019

Uh oh!

alekla01 commented Feb 8, 2019

Uh oh!

SeppoTakalo commented Feb 8, 2019

Uh oh!

Uh oh!

SeppoTakalo commented Jan 18, 2019 •

edited

Loading