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

Allows multiple network status listeners #9424

Merged
merged 7 commits into from Feb 11, 2019

Conversation

@SeppoTakalo
Copy link
Contributor

commented Jan 18, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

@ciarmcom ciarmcom requested review from kjbracey-arm, teetak01, yogpan01 and ARMmbed/mbed-os-maintainers Jan 18, 2019

features/netsocket/NetworkInterface.cpp Outdated
}
}

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

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Jan 18, 2019

Contributor

How about if you give NULL here as status_cb?

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 18, 2019

Author Contributor

Why would you?

It is not acceptable value and leads to HardFault.

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Jan 18, 2019

Author Contributor

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 SeppoTakalo force-pushed the SeppoTakalo:ONME-4125 branch Jan 29, 2019

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

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

This work is now complete and no blocker issues.

Can be tested and merged in.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

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

Can you try 'deregister'?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

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
Copy link
Contributor

left a comment

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

  1. 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);
  1. 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)

SeppoTakalo added some commits Jan 17, 2019

Allow multiple network status listeners
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
->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 SeppoTakalo force-pushed the SeppoTakalo:ONME-4125 branch to e9f5ed4 Jan 31, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@michalpasztamobica Added the unittest that you proposed.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

Thank you, review reapproved after changes..

Call "new" instead of no-throw version and the MBED_ERROR.
Standard new operator already calls MBED_ERROR in failure.
@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

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

@SeppoTakalo SeppoTakalo force-pushed the SeppoTakalo:ONME-4125 branch Feb 4, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Checked with Kevin.B.
Should be fixed now.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

@0xc0170 can be tested now.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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 SeppoTakalo force-pushed the SeppoTakalo:ONME-4125 branch to 43a53df Feb 6, 2019

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

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

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

CI started

@0xc0170 0xc0170 requested a review from AnotherButler Feb 7, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

@alekla01 Thanks for the clarification.

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

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+8 bytes) RAM(+48 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9812 cycles (+56 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.