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

Update ODIN drivers to v3.0.0 RC1 #7579

Merged
merged 1 commit into from Sep 3, 2018

Conversation

Projects
None yet
@asifrizwanubx
Contributor

asifrizwanubx commented Jul 23, 2018

Description

This release contains the following new features.

  • Access Point functionality
  • Bridge functionality
  • Robustness

This release contains the following fixes.

Known limitations

  • Only possible to use either Wi-Fi AP or Wi-Fi Station

Test Results

The following tests have been performed

  • mbed-os test for GCC_ARM, ARM and IAR
  • mbed-os WiFi tests (target for Robustness)

iar_network_wifi_logs.txt
arm_mbed_logs.txt
arm_network_wifi_logs.txt
gcc_mbed_logs.txt
gcc_network_wifi_logs.txt
iar_mbed_logs.txt

Other

  • To enable the bridge add the below macro and increase the lwIP pbuf pool size in mbed_app.json, for example:
    "macros": ["MBED_EMAC_LWIP_L2_BRIDGE=1"],
    "lwip-pbuf-pool-size":
    { "help": "Number of pbuf pool buffers", "value": "80" }
  • To enable the AP add the below macro in mbed_app.json, for example:
    "macros": ["DEVICE_WIFI_AP=1"],

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change
@mlubinsky

This comment has been minimized.

mlubinsky commented Jul 23, 2018

How to get this patch #7579
into my project, which is a fork of
https://github.com/armmbed/ref-wem-firmware
?
I am using mbed-cli. I need to test it ASAP.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2018

@mlubinsky Fetch this via remote. One of the approaches could be

git checkout -b u-blox-ublox_odin_driver_os_5_v3.0.0_rc1 master
git pull https://github.com/u-blox/mbed-os.git ublox_odin_driver_os_5_v3.0.0_rc1
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 25, 2018

I can see now why this was marked as feature. First two commits : one is fixing tests, another one is adding new feature. I believe this 2 shall be sent separately (odin update driver would depend on them).

Can you split this PR please

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Jul 26, 2018

revert the changes related to wifi tests, will be push in a seperate PR. @0xc0170

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Jul 26, 2018

@AmmadRehmat could you please create a new PR for fix of WiFi tests?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jul 26, 2018

@asifrizwanubx
Consider enabling all network tests.

Building project tcp (UBLOX_EVK_ODIN_W2, GCC_ARM)
Scan: GCC_ARM
Scan: tcp
Compile [  8.3%]: main.cpp
[Error] main.cpp@19,2: #error [NOT_SUPPORTED] No network configuration found for this target.
[Error] main.cpp@59,25: 'MBED_CONF_APP_CONNECT_STATEMENT' was not declared in this scope
[Error] main.cpp@72,36: 'MBED_CONF_APP_ECHO_SERVER_ADDR' was not declared in this scope
[Error] main.cpp@73,23: 'MBED_CONF_APP_ECHO_SERVER_PORT' was not declared in this scope
[Error] main.cpp@82,36: 'MBED_CONF_APP_ECHO_SERVER_ADDR' was not declared in this scope
Building project stats_heap (UBLOX_EVK_ODIN_W2, GCC_ARM)

In order to run TCP and UDP tests you need to provide test configs in mbed_app.json or as a test config.json. Look for samples in mbed-os/tools/test-configs

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jul 26, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 26, 2018

revert the changes related to wifi tests, will be push in a seperate PR. @0xc0170

Thanks, is adding WiFiSoftAPInterface.h is part of this PR as it relates to the driver update ?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jul 26, 2018

I don't see where this flag DEVICE_WIFI_AP is defined or documented.

Also, I don't much like this #ifdeffing that changes the class totally.

Why would you not have separate class that implements the AP functions on top of UbloxOdinInterface?

So instead of

#ifdef DEVICE_WIFI_AP
+class OdinWiFiInterface : public WiFiInterface, public WiFiSoftAPInterface, public EMACInterface
#else
class OdinWiFiInterface : public WiFiInterface, public EMACInterface
#endif

You would do:

class OdinWiFiInterface : public WiFiInterface, public EMACInterface {
...
};
class OdinSoftAPInterface: public OdinWifiInterface, public WifiSoftAPInterface {
 // Soft AP functions here
}

Would allow you to get rid of #ifdef and still allows linker to drop unused AP functions. (Unless there is some runtime checks in your .cpp code that are disabled by #ifndef DEVICE_WIFI_AP)

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Jul 27, 2018

@0xc0170

is adding WiFiSoftAPInterface.h is part of this PR as it relates to the driver update ?

yes it is part of this PR without it driver with AP functionality didn't work.

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Jul 27, 2018

@SeppoTakalo

I don't see where this flag DEVICE_WIFI_AP is defined or documented.

We have added the description of DEVICE_WIFI_AP in PR and by this macro we will restrict the application use of station and AP at a time. Otherwise it will not work.

We will deliver soon a public example for AP with proper documentation.

Could you please point out where will I add description of DEVICE_WIFI_AP?

@screamerbg

This comment has been minimized.

Member

screamerbg commented Aug 1, 2018

@SeppoTakalo do you have further requests/suggestions?

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Aug 1, 2018

I need @kjbracey-arm and @mikaleppanen to review this, but Kevin is still on vacation.

This PR adds WifiSoftAPInterface as part of public Mbed OS API so it needs more eyes and thoughts. We need to decide how to document and support this after it becomes part of Mbed OS.

@AmmadRehmat

This comment has been minimized.

Contributor

AmmadRehmat commented Aug 7, 2018

@SeppoTakalo
Attaching test reports for tcp and udp tests.
tcp.log
udp.log

Consider enabling all network tests.

Building project tcp (UBLOX_EVK_ODIN_W2, GCC_ARM)
Scan: GCC_ARM
Scan: tcp
Compile [ 8.3%]: main.cpp
[Error] main.cpp@19,2: #error [NOT_SUPPORTED] No network configuration found for this target.
[Error] main.cpp@59,25: 'MBED_CONF_APP_CONNECT_STATEMENT' was not declared in this scope
[Error] main.cpp@72,36: 'MBED_CONF_APP_ECHO_SERVER_ADDR' was not declared in this scope
[Error] main.cpp@73,23: 'MBED_CONF_APP_ECHO_SERVER_PORT' was not declared in this scope
[Error] main.cpp@82,36: 'MBED_CONF_APP_ECHO_SERVER_ADDR' was not declared in this scope
Building project stats_heap (UBLOX_EVK_ODIN_W2, GCC_ARM)

In order to run TCP and UDP tests you need to provide test configs in mbed_app.json or as a test config.json. Look for samples in mbed-os/tools/test-configs

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Aug 7, 2018

@AmmadRehmat

This comment has been minimized.

Contributor

AmmadRehmat commented Aug 7, 2018

Attaching extended testing reports.
udp_extended_log.txt
tcp_extended_log.txt

Please note that you can enable full set of TCP+UDP tests by enabling macro MBED_EXTENDED_TESTS

https://github.com/ARMmbed/mbed-os/blob/master/TESTS/netsocket/tcp/main.cpp#L137
https://github.com/ARMmbed/mbed-os/blob/master/TESTS/netsocket/udp/main.cpp#L94

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Aug 7, 2018

Looks good.

@0xc0170

Cosmetic changes only

#include "emac_lwip_l2_bridge.h"
#include "string.h"
#include "mbed.h"

This comment has been minimized.

@0xc0170

0xc0170 Aug 7, 2018

Member

Can this be replaced by header files that are required here? Rather than one header file mbed.h

This comment has been minimized.

@asifrizwanubx

asifrizwanubx Aug 9, 2018

Contributor

done

@@ -0,0 +1,413 @@
/* mbed Microcontroller Library
* Copyright (c) 2016 ARM Limited

This comment has been minimized.

@0xc0170

0xc0170 Aug 7, 2018

Member

2018 year for new files

void LWIPMemoryManager::ref(emac_mem_buf_t *buf)
{
pbuf_ref(static_cast<struct pbuf *>(buf));
}

This comment has been minimized.

@0xc0170

0xc0170 Aug 7, 2018

Member

Wasnt there a new line at the end of this line? Github shows there is none now, this results in warning in some toolchains

This comment has been minimized.

@AmmadRehmat

AmmadRehmat Aug 8, 2018

Contributor

Since we are working on cosmetic changes, I have noticed that in release 5.9.0, feature_lwip folder contains files which have been moved to lwipstack folder. In 5.9.3 we only have 2 files in folder feature_lwip, emac_lwip_l2_bridge.h and emac_lwip_l2_bridge.c. Should these files be moved to lwipstack folder as well?

err_t res;
bool free_msg = true;
_mem.ref(buf);

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 8, 2018

Contributor

Reference counting has not been added to general EMAC memory manager interface (EMACMemoryManager.h), but since it is needed only for this lwip specific brigde, I'm not against implementing it only to lwip memory manager for now.

* @param ssid Name of the network to connect to
* @param pass Security passphrase to connect to the network
* @param security Type of encryption for connection (Default: NSAPI_SECURITY_NONE)
* @param channel Channel on which the connection is to be made, or 0 for any (Default: 0)

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 8, 2018

Contributor

Is there support for "0" setting? Seems to fail in is_valid_AP_channel() with channel set to zero.

This comment has been minimized.

@AmmadRehmat

AmmadRehmat Aug 8, 2018

Contributor

The description is a little bit misleading, we will fix the description. Channel zero is not allowed but error handling for non supporting channels is available. Will fix the description soon.

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 9, 2018

Contributor

There are also similar descriptions about channel 0 in OdinWiFiInterface.h and in set_ap_channel() function call. They should be updated too.

This comment has been minimized.

@asifrizwanubx

asifrizwanubx Aug 9, 2018

Contributor

done

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 9, 2018

@0xc0170 @mikaleppanen Mind re-reviewing?

@cmonr cmonr added needs: review and removed needs: work labels Aug 9, 2018

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Aug 9, 2018

@mikaleppanen @0xc0170 please review it again. I made the changes according to your feedback.

nsapi_error_t error_code = _stack.add_ethernet_interface(_emac, true, &_interface);
if (error_code != NSAPI_ERROR_OK) {
_interface = NULL;
}

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 10, 2018

Contributor

Like in EMACInterface::connect(), there needs to be call here to "_interface->attach(_connection_status_cb);" in else branch to add the callback to interface. Otherwise callbacks attached between the construct and connect() do not work.

This comment has been minimized.

@asifrizwanubx

asifrizwanubx Aug 10, 2018

Contributor

is #6562 issue due to this?

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 10, 2018

Contributor

I think that #7310 could be caused by this.

@AmmadRehmat

This comment has been minimized.

Contributor

AmmadRehmat commented Aug 31, 2018

@kjbracey-arm commit squashed and cleaned.

@cmonr cmonr added needs: CI and removed needs: work labels Aug 31, 2018

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Sep 1, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 1, 2018

needs: review again. Needs @kjbracey-arm approval

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 2, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 2, 2018

@kjbracey-arm @mikaleppanen #7579 (comment)

Re-requesting that this PR be re-reviewewed. Comments look to be addressed.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 3, 2018

Interesting. Test failures don't look related to PR.
/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 44925d8 into ARMmbed:master Sep 3, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 563 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9881 cycles (-15 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 3, 2018

@0xc0170 0xc0170 changed the title from Updated ODIN drivers to v3.0.0 RC1 to Update ODIN drivers to v3.0.0 RC1 Sep 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

@flipflop22

This comment has been minimized.

flipflop22 commented Oct 1, 2018

I have been trying to use this AP feature. It does not work well.
I can see the Network is there, however I can't connect a station (another ODIN-W2) to the AP

@asifrizwanubx

This comment has been minimized.

Contributor

asifrizwanubx commented Oct 1, 2018

@flipflop22

Could you please share the error or issue?

Or try this example.
https://os.mbed.com/teams/ublox/code/mbed-os-example-odinw2-wifi-ap/

If still you face some issue please report it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment