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

Add WIFI support for RDA target UNO_91H #9501

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
@caixue1102
Copy link
Contributor

caixue1102 commented Jan 25, 2019

Description

Add support wifi for RDA target UNO_91H.
Tested with GCC_ARM, IAR and ARMCC.
Test report here:
RDA_UNO_91H_test_report.zip

Pull request type

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

Reviewers

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 25, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 25, 2019

@0xc0170
Copy link
Member

0xc0170 left a comment

Looks fine, just that I would run astyle for implementation files of Mbed OS functionality (like emac port , or wifi interface - RdaWiFiInterface).

+1 for providing licenses for libs

@SeppoTakalo
Copy link
Contributor

SeppoTakalo left a comment

Driver looks OK, but I'm requesting changes for couple of things.

  • License header copy&pasted from other files and not updated to reflect this work. Now headers don't reflect the original author of these files, but instead say Copyright Arm
  • Small code style changes requested.
  • Usage of mbed_mac_address() without providing implementation for it.
  • Copied LwIP headers without clear explanation of why
  • tab used as indentation marker in targets.json file.
}
}
} else
find = true;

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Our style guide requires one line if-else clauses to have brackets as well.

features/netsocket/emac-drivers/TARGET_RDA_EMAC/RdaWiFiInterface.cpp Outdated
find = true;

if (find == false) {
printf("can not find the ap.\r\n");

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Use mbed_trace or other loggers.
I don't think using printf() inside driver is acceptable, as you cannot disable the output from build time.

features/netsocket/emac-drivers/TARGET_RDA_EMAC/RdaWiFiInterface.cpp Outdated
_blocking);

if (ret == NSAPI_ERROR_DHCP_FAILURE)
ret = NSAPI_ERROR_CONNECTION_TIMEOUT;

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Why do you translate DHCP_FAILURE to CONNECTION_TIMEOUT?



nsapi_error_t RDAWiFiInterface::connect()
{

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

I don't see the point why this cannot call RDAWiFiInterface::connect(const char *ssid, const char *paswd, nsapi_security_t security, uint8_t channel)

features/netsocket/emac-drivers/TARGET_RDA_EMAC/RdaWiFiInterface.cpp Outdated
if(sta_state < 2)
return NSAPI_ERROR_NO_CONNECTION;

init();

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Why do you call init() here?

I would have assumed you to call power_down() or equivalent.

features/netsocket/emac-drivers/TARGET_RDA_EMAC/RdaWiFiInterface.h Outdated
@@ -0,0 +1,135 @@
/* LWIP implementation of NetworkInterfaceAPI
* Copyright (c) 2019 ARM Limited

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Same here, as previous.. You should not copy&paste the license section.

.../netsocket/emac-drivers/TARGET_RDA_EMAC/lwip-wifi/arch/TARGET_RDA/TARGET_UNO_91H/inc/dhcps.h Outdated
*/

#ifndef __DHCPS_H__
#define __DHCPS_H__

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Why this file is copied from LwIP?

...et/emac-drivers/TARGET_RDA_EMAC/lwip-wifi/arch/TARGET_RDA/TARGET_UNO_91H/inc/lwipopts_conf.h Outdated


#ifndef LWIPOPTS_CONF_H
#define LWIPOPTS_CONF_H

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Why this file is copied from LwIP?


bool RDA5981x_EMAC::get_hwaddr(uint8_t *addr) const
{
mbed_mac_address((char *)addr);

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

I don't see this function used anywhere else in the driver, are you sure that it actually provides the MAC address that this hardware uses?

If you don't provide implementation of mbed_mac_address() it gives you pseudo MAC address that is generated in build time.

targets/targets.json Outdated
"ANALOGIN",
"FLASH",
"TRNG"
"USTICKER",

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 25, 2019

Contributor

Tabulator used as indentation here.. Use spaces as the rest of the file.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Jan 25, 2019

Also, I would like to see test results as instructed in https://os.mbed.com/docs/mbed-os/v5.11/porting/wifi-port.html#testing

So provide log file from mbed test command running all network socket tests as well as WiFi tests.

See the Socket test plan what kind of mbed_app.json is required to run it properly.

@caixue1102 caixue1102 force-pushed the caixue1102:mbed-os-rda branch 2 times, most recently Jan 28, 2019

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Jan 28, 2019

thank you for the suggestions。

  1. coding style changes are made according to your suggestions.
  2. return values in connect function have been modified by determining the cause of the connection errors.
  3. you are right, we finally called RDAWiFiInterface::connect(const char *ssid, const char *paswd, nsapi_security_t security, uint8_t channel) in RDAWiFiInterface::connect();
  4. init() is useless in disconnect, so it has been removed.
  5. Copyright has been modified in all the files.
  6. LwIP headers are useless and they were removed.
  7. mbed_mac_address is used in wifi library function, and final used by the hardware.
@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Jan 30, 2019

Also, I would like to see test results as instructed in https://os.mbed.com/docs/mbed-os/v5.11/porting/wifi-port.html#testing

So provide log file from mbed test command running all network socket tests as well as WiFi tests.

See the Socket test plan what kind of mbed_app.json is required to run it properly.

The test report pasted in the description is runned on the newest TEST case (netsocket and network)with 'mbed test' command , is there anything wrong with my operation? could you please give me some pointers?

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Jan 30, 2019

@SeppoTakalo please help to review the new changes, thank you very much~

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Jan 30, 2019

@caixue1102 Sorry, I did not spot the attached log files.

Those are fine and look OK.

features/netsocket/emac-drivers/TARGET_RDA_EMAC/RdaWiFiInterface.cpp Outdated
{
#if 1
return connect(_ssid, _pass, _security, _channel);
#else

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jan 30, 2019

Contributor

In the future, you might want to remove the dead code from the driver.
When left there, it soon gets outdated, and might be misleading for the next developer who tries to understand this driver.

This comment has been minimized.

@caixue1102

caixue1102 Jan 30, 2019

Author Contributor

you are right, and I have remove the dead code just now.

@caixue1102 caixue1102 force-pushed the caixue1102:mbed-os-rda branch Jan 30, 2019

features/netsocket/emac-drivers/TARGET_RDA_EMAC/rda5981x_emac.cpp Outdated
}

}
else

This comment has been minimized.

@0xc0170
Copy link
Member

0xc0170 left a comment

2 requests, the rest looks fine

  • SPDX identifier addition
  • fixing coding style in the implementation files (emac/wifiinterface)
@@ -0,0 +1,160 @@
/* Copyright (c) 2019 Unisoc Communications Inc.
*

This comment has been minimized.

@0xc0170

0xc0170 Jan 30, 2019

Member

can you add SPDX identifier to new files please?

 * SPDX-License-Identifier: Apache-2.0

@caixue1102 caixue1102 force-pushed the caixue1102:mbed-os-rda branch Jan 31, 2019

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Jan 31, 2019

@0xc0170 @VeijoPesonen Thank you, now coding style is fixed and SPDX identifier is added.

@cmonr cmonr added needs: CI and removed needs: work labels Jan 31, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 31, 2019

CI started

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Feb 11, 2019

@0xc0170 thank you, this has been modified

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 11, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 11, 2019

Test run: FAILED

Summary: 6 of 8 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 11, 2019

Please review build artifacts, there are failures:

[DEBUG] Output: "./cmsis/TARGET_CORTEX_M/mbed_fault_handler.c", line 23: Error: #5: cannot open source input file "device.h": No such file or directory

@caixue1102 caixue1102 force-pushed the caixue1102:mbed-os-rda branch Feb 12, 2019

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Feb 12, 2019

@0xc0170 Thank you,I have modified target.json and now it can be compiled successfully using -m RDA5981X, I didn't find the failed reason before, because I used -m UNO_91H for all tests in the past.and now using -m RDA5981X can not get a emac.bin.

@cmonr cmonr added needs: CI and removed needs: work labels Feb 12, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 12, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 12, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 12, 2019

targets/targets.json Outdated
"ANALOGIN",
"FLASH",
"TRNG"
"USTICKER",

This comment has been minimized.

@0xc0170

0xc0170 Feb 12, 2019

Member

I've noticed this should be aligned as it was previously (4 spaces more to the right) ?

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Feb 12, 2019

@caixue1102 Please send a new PR fixing the alignment or amend it here, let us know (will start CI)

@0xc0170 0xc0170 added needs: work and removed ready for merge labels Feb 12, 2019

@caixue1102 caixue1102 force-pushed the caixue1102:mbed-os-rda branch to d92e33d Feb 12, 2019

@caixue1102

This comment has been minimized.

Copy link
Contributor Author

caixue1102 commented Feb 12, 2019

@0xc0170 thank you and it's modified.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 12, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Feb 12, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 12, 2019

Test run: FAILED

Summary: 3 of 8 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
@alekla01

This comment has been minimized.

Copy link
Contributor

alekla01 commented Feb 12, 2019

Restarted CI after #9677

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Feb 12, 2019

Test run: SUCCESS

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

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 12, 2019

@cmonr cmonr merged commit 3e6cd4e into ARMmbed:master Feb 13, 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(+0 bytes) RAM(+0 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 10196 cycles (+967 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

@cmonr cmonr removed the ready for merge label Feb 13, 2019

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.