Remove string-based API usage within mbed-os #11941

michalpasztamobica · 2019-11-25T20:53:06Z

Description

Summary of change

Remove internal usage of deprecated string-based APIs.
The APIs were deprecated in #11914
This is still NOT a breaking change. The existing code was updated not to use the deprecated API functions, but the deprecated functions themselves are still available.

Documentation

The documentation was updated in the deprecation PR. Wherever new API had to be added I adjusted existing documentation.

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@AnttiKauppila
@SeppoTakalo
@mikter
@AriParkkila

Release Notes

See #11914 for deprecation information, this PR provides SocketAddress-based APIs if they were missing after previous changes, but still does not remove the old APIs.

Summary of changes

New SocketAddress based API added to ESP8266 module, anticipating future string-based API removal.

Impact of changes

Migration actions required

michalpasztamobica · 2019-11-25T20:56:29Z

@0xc0170 , again, I could not run all possible platforms, but at least I checked unittests and some combinations of UBLOX/K64F with different compilers. I'd be grateful for starting a CI, as the API is scatterred and some extra changes might be required.
I did change the NetworkStack.cpp file, but did not change the API, so I hope the wiced binaries do not need a rebuild.
Finally, is it possible for this PR to still make it into mbed-os-5.15?

ciarmcom · 2019-11-25T22:00:26Z

@michalpasztamobica, thank you for your changes.
@AnttiKauppila @mikter @AriParkkila @SeppoTakalo @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

michalpasztamobica · 2019-11-26T05:15:18Z

Pushed astyle fixes.

AnttiKauppila · 2019-11-26T07:11:12Z

components/wifi/esp8266-driver/ESP8266Interface.h

@@ -137,6 +137,8 @@ class ESP8266Interface : public NetworkStack, public WiFiInterface {
 /** Get the internally stored IP address
 * @return IP address of the interface or null if not yet connected
 */
+ virtual nsapi_error_t get_ip_address(SocketAddress *address);
+
 virtual const char *get_ip_address();


Should we mark these as deprecated as well?

I pushed an update deprecating string-based API of ESP8266 driver. I think this is a matter of being consequent. We cannot forbid member function overloading, but at least we can give a good example.

Another thing is that users will most likely use NetworkInterface::get_default_instance() rather that consciously creating EPS8266Interface, so they should be given deprecation warnings if they use string-based APIs anyway?

AnttiKauppila

We might want to drop string based functions also from drivers (at least from those we have made), but that can be done later

0xc0170 · 2019-11-26T09:10:40Z

CI started

mbed-ci · 2019-11-26T11:16:02Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

michalpasztamobica · 2019-11-26T12:17:03Z

I will investigate the failure.

michalpasztamobica · 2019-11-26T20:11:27Z

With the SocketAddress API some test cases make no sense (like empty string as hostname), so I simplified the tlssocket_connect_invalid test.

Regarding udpsocket_sendto_invalid, @kjbracey-arm , if we use a SocketAddress, set to an invalid address (SocketAddress.set_ip_address("")) what should sendto return? LWIP seems to return 0, while ism43362 returns NSAPI_ERROR_DEVICE_ERROR. I relaxed the test's expectation to accept any error code or 0 (for 0 bytes sent). Does this make sense to you?

I put the above changes into separate commits.

For DISCO_L475VG_IOT01A, I had to create a PR in wifi-ism43362 repository: ARMmbed/wifi-ism43362#65 . I am afraid we won't be able to progress here before it gets merged. @0xc0170 , can we restart CI just to see if other platforms got fixed?

For the NULCEO_F429ZI failures - I don't know what is going on. I run the tests locally and they all passed. The logs also do not indicate any particular issue...

0xc0170 · 2019-11-27T07:58:07Z

CI started

mbed-ci · 2019-11-27T09:27:15Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

michalpasztamobica · 2019-11-27T09:43:49Z

OK. The only failures left are coming from DISCO_L475VG_IOT01A. They will pass as soon as ARMmbed/wifi-ism43362#65 is reviewed and merged.

0xc0170 · 2019-11-27T09:45:02Z

OK. The only failures left are coming from DISCO_L475VG_IOT01A. They will pass as soon as ARMmbed/wifi-ism43362#65 is reviewed and merged.

If this needs an update like that, is this breaking change?

michalpasztamobica · 2019-11-27T09:48:40Z

@AnttiKauppila , @0xc0170 I am slightly afraid that this PR might wreak havoc in the nightly tests... Not sure how many different platform use external stacks?
@0xc0170 - this PR will not break an existing application, but our tests (greentea/unittests) are now using a new stack interface, so they might get broken if an external stack is used which does not implement those functions (for example ism43362). Does that count as a "breaking change"?

0xc0170 · 2019-11-27T12:26:29Z

ARMmbed/wifi-ism43362#65

Once it's merged, lets start CI again. Set as preceding PR needed here

0xc0170 · 2019-11-27T13:24:24Z

CI restarted

mbed-ci · 2019-11-27T14:41:49Z

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

0xc0170 · 2019-11-27T15:32:13Z

@michalpasztamobica Please can you review the failures?

michalpasztamobica · 2019-11-27T15:46:35Z

@0xc0170 , I am working on it...

michalpasztamobica · 2019-11-27T17:30:50Z

Root cause: wifi-ism43362 driver's socket_sendto function hangs when compiled with IAR and given an invalid SocketAddress as a parameter. I will filed an issue in wifi-ism43362 github project: ARMmbed/wifi-ism43362#66

We can however proceed with the release, because after some more thinking I decided to remove udpsocket_sendto_invalid from test suite.
The purpose of udpsocket_sendto_invalid was to check if our protections against invalid hostname (NULL ptr or empty string) are working. The "hostname+port" version did check for parameter validity.
The SocketAddress version of InternetDatagramSocket::sendto does NOT check address parameter's validity and it never did. The reason is probably that SocketAddress class has its own mechanism for checking address validity, that user should make use of.
By adjusting the existing udpsocket_sendto_invalid test I have treaded into undefined behavior territory and now the test in fact checked error handling of underlying network stacks (and found a bug in one...).

If I added SocketAddress validity checks to InternetDatagramSocket::sendto this would make this PR a breaking change, and that's not at all my goal here. Removing the test which was testing deprecated API is the right thing to do.

I added an internal JIRA to find out what is the expected behavior of InternetDatagramSocket::sendto in case the SocketAddress is invalid and test against it.

@0xc0170 , please restart the CI, unless you see some gap in my way of thinking?
@AnttiKauppila , @bulislaw , please confirm that test removal makes sense to you as well.

Updated: * netsocket classes, * unittests, stubs and mocks, * greentea tests

…and get_netmask

It tested parameter checks in the now obsoleted string-based API.

michalpasztamobica · 2019-11-27T18:07:30Z

I pushed a cleaned-up commit history and test documentation updates.

michalpasztamobica · 2019-11-28T09:06:36Z

Is everyone OK with the removal of the test? Can we move forward with this?

SeppoTakalo

I'm OK with the change.

However, I did not spot, do we still have test case where we bind() on a socket without specifying the address, just the port number?

SocketAddress port;
port.set_port(80);
socket.bind(port);  // Should not require you to specify IP address.
socket.listen();
s = socket.accept();

michalpasztamobica · 2019-11-28T10:18:27Z

Yes. We do have TCPSOCKET_BIND_PORT and USPSOCKET_BIND_PORT.

0xc0170 · 2019-11-28T10:22:08Z

I started CI meanwhile

mbed-ci · 2019-11-28T11:47:55Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 4
Build artifacts

michalpasztamobica · 2019-11-28T12:07:28Z

@AnttiKauppila , would you confirm that you're ok with the recent changes?

michalpasztamobica · 2019-11-28T13:21:58Z

Hi @0xc0170 , just to sum up the situation, as it got a bit complex with all the CI failures:
The CI passed after removing the test. The removal is justified and approved by Antti and Seppo.
All the bugs found are being tracked elsewhere.

If there is anything more that I can do to improve this PR, please let me know :).

0xc0170 · 2019-11-28T13:25:40Z

@michalpasztamobica Thanks for the updates, keeps us all update about the progress 👍

kivaisan · 2019-11-28T13:42:08Z

features/netsocket/PPPInterface.cpp

@@ -92,6 +92,41 @@ const char *PPPInterface::get_ip_address()
 return NULL;
 }

+nsapi_error_t PPPInterface::get_ip_address(SocketAddress *address)
+{
+ if (address) {


Sorry, I didn't notice this earlier, but there's a bug in here. Should be if (!address) {

Thanks for pointing this out, @kivaisan .
PR that addresses this is redy: #11975 . @0xc0170 , sorry for this disturbance, but could we merge that PR, so the release has bug free code?

The reason why this was not spotted by CI is because the tests are pending: #11878

ciarmcom requested review from AnttiKauppila, AriParkkila, mikter, SeppoTakalo and a team November 25, 2019 22:00

ciarmcom added the needs: review label Nov 25, 2019

michalpasztamobica force-pushed the remove_internal_string_apis branch from b858fad to 19c9761 Compare November 26, 2019 05:13

AnttiKauppila reviewed Nov 26, 2019

View reviewed changes

AnttiKauppila approved these changes Nov 26, 2019

View reviewed changes

michalpasztamobica force-pushed the remove_internal_string_apis branch from 19c9761 to 1de97e9 Compare November 26, 2019 07:48

0xc0170 added the release-version: 5.15.0-rc1 label Nov 26, 2019

0xc0170 approved these changes Nov 26, 2019

View reviewed changes

michalpasztamobica mentioned this pull request Nov 26, 2019

IPCore String-based API removal #11942

Merged

0xc0170 added needs: work and removed needs: review labels Nov 26, 2019

michalpasztamobica added 4 commits November 27, 2019 20:02

Remove internal usage of deprecated APIs in IPCore

ea04c4f

Updated: * netsocket classes, * unittests, stubs and mocks, * greentea tests

ESP8266: add SocketAddress-based API for get_ip_address, get_gateway …

83cd0d6

…and get_netmask

Tests: TLSSocket invalid connect simplified

5ece00c

Remove udpsocket_sendto_invalid

71db612

It tested parameter checks in the now obsoleted string-based API.

michalpasztamobica force-pushed the remove_internal_string_apis branch from 72ea0e9 to 71db612 Compare November 27, 2019 18:07

SeppoTakalo approved these changes Nov 28, 2019

View reviewed changes

0xc0170 added needs: review and removed needs: preceding PR needs: work labels Nov 28, 2019

michalpasztamobica requested a review from AnttiKauppila November 28, 2019 12:31

AnttiKauppila approved these changes Nov 28, 2019

View reviewed changes

0xc0170 removed the needs: review label Nov 28, 2019

0xc0170 merged commit ffdd543 into ARMmbed:master Nov 28, 2019

michalpasztamobica deleted the remove_internal_string_apis branch November 28, 2019 13:26

kivaisan reviewed Nov 28, 2019

View reviewed changes

michalpasztamobica mentioned this pull request Nov 28, 2019

PPPinterface: remove faulty address getter check for invalid pointer #11975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove string-based API usage within mbed-os #11941

Remove string-based API usage within mbed-os #11941

michalpasztamobica commented Nov 25, 2019

michalpasztamobica commented Nov 25, 2019

ciarmcom commented Nov 25, 2019

michalpasztamobica commented Nov 26, 2019

AnttiKauppila Nov 26, 2019

michalpasztamobica Nov 26, 2019

AnttiKauppila left a comment

0xc0170 commented Nov 26, 2019

mbed-ci commented Nov 26, 2019

michalpasztamobica commented Nov 26, 2019

michalpasztamobica commented Nov 26, 2019

0xc0170 commented Nov 27, 2019

mbed-ci commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

0xc0170 commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

0xc0170 commented Nov 27, 2019 •

edited

Loading

0xc0170 commented Nov 27, 2019

mbed-ci commented Nov 27, 2019

0xc0170 commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019 •

edited

Loading

michalpasztamobica commented Nov 27, 2019

michalpasztamobica commented Nov 28, 2019

SeppoTakalo left a comment •

edited

Loading

michalpasztamobica commented Nov 28, 2019 •

edited

Loading

0xc0170 commented Nov 28, 2019

mbed-ci commented Nov 28, 2019

michalpasztamobica commented Nov 28, 2019

michalpasztamobica commented Nov 28, 2019

0xc0170 commented Nov 28, 2019

kivaisan Nov 28, 2019

michalpasztamobica Nov 28, 2019

Remove string-based API usage within mbed-os #11941

Remove string-based API usage within mbed-os #11941

Conversation

michalpasztamobica commented Nov 25, 2019

Description

Summary of change

Documentation

Pull request type

Test results

Reviewers

Release Notes

Summary of changes

Impact of changes

Migration actions required

michalpasztamobica commented Nov 25, 2019

ciarmcom commented Nov 25, 2019

michalpasztamobica commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnttiKauppila left a comment

Choose a reason for hiding this comment

0xc0170 commented Nov 26, 2019

mbed-ci commented Nov 26, 2019

michalpasztamobica commented Nov 26, 2019

michalpasztamobica commented Nov 26, 2019

0xc0170 commented Nov 27, 2019

mbed-ci commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

0xc0170 commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

0xc0170 commented Nov 27, 2019 • edited Loading

0xc0170 commented Nov 27, 2019

mbed-ci commented Nov 27, 2019

0xc0170 commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019

michalpasztamobica commented Nov 27, 2019 • edited Loading

michalpasztamobica commented Nov 27, 2019

michalpasztamobica commented Nov 28, 2019

SeppoTakalo left a comment • edited Loading

Choose a reason for hiding this comment

michalpasztamobica commented Nov 28, 2019 • edited Loading

0xc0170 commented Nov 28, 2019

mbed-ci commented Nov 28, 2019

michalpasztamobica commented Nov 28, 2019

michalpasztamobica commented Nov 28, 2019

0xc0170 commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Nov 27, 2019 •

edited

Loading

michalpasztamobica commented Nov 27, 2019 •

edited

Loading

SeppoTakalo left a comment •

edited

Loading

michalpasztamobica commented Nov 28, 2019 •

edited

Loading