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

[Wio 3G] Adding platform #7098

Merged
merged 3 commits into from Jun 14, 2018

Conversation

Projects
None yet
8 participants
@ytsuboi
Contributor

ytsuboi commented Jun 4, 2018

Description

Adding new platform Wio 3G, STM32F439VI with Quectel UG96 on-board 3G modem.

I don't want to use preprocessor macro to switch AT command. But since member function of cellular feature such as set_context_to_be_activated() are private, I decided to use macro to reduce shared code changes ( especially changing code structure ).

Test results:
GCC
armcc
EWARM

Pull request type

[ ] Fix
[ ] Refactor
[x] New target
[ ] Feature
[ ] Breaking change
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 4, 2018

@ytsuboi only GCC ARM test results provided?

@ytsuboi

This comment has been minimized.

Contributor

ytsuboi commented Jun 5, 2018

@0xc0170 Added on description.
@toyowata Thanks for your help!

"config": {
"clock_source": {
"help": "Mask value : USE_PLL_HSE_EXTC | USE_PLL_HSE_XTAL (need HW patch) | USE_PLL_HSI",
"value": "USE_PLL_HSE_XTAL|USE_PLL_HSI",

This comment has been minimized.

@ashok-rao

ashok-rao Jun 5, 2018

Contributor

Question: What is the HW patch? Is this explained somewhere in the docs?

This comment has been minimized.

@ytsuboi

ytsuboi Jun 5, 2018

Contributor

Oops, no need hardware patch. XTAL is mounted on the board. Will remove "need HW patch" on next commit.

This comment has been minimized.

@ytsuboi

ytsuboi Jun 8, 2018

Contributor

Done

USB_OTG_HS_ULPI_DIR = PC_2,
USB_OTG_HS_ULPI_NXT = PC_3,
*/

This comment has been minimized.

@ashok-rao

ashok-rao Jun 5, 2018

Contributor

I think all of these un-used USB pins can be removed..

This comment has been minimized.

@ytsuboi

ytsuboi Jun 8, 2018

Contributor

Done

LED_RED = LED2,
USER_BUTTON = PC_13,
// Standardized button names
// BUTTON1 = USER_BUTTON,

This comment has been minimized.

@ashok-rao

ashok-rao Jun 5, 2018

Contributor

This one is required for consistency.

This comment has been minimized.

@ytsuboi

ytsuboi Jun 8, 2018

Contributor

Done

@ashok-rao

LGTM. Thanks, nice work!

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jun 5, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 5, 2018

@ARMmbed/mbed-os-wan Can you review cellular additions here?

@@ -429,11 +429,21 @@ nsapi_error_t AT_CellularNetwork::set_context_to_be_activated()
// if user has defined user name and password we need to call CGAUTH before activating or modifying context
if (_pwd && _uname) {
#if defined(TARGET_WIO_3G)

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 5, 2018

Contributor

This is not a correct approach. Now I understand the context of your earlier question about set_context_to_be_activated accessibility. I agree it does not make sense to copy paste activate_context method to your own class.
Please propose a solution where set_context_to_be_activated is changed to protected and virtual method, so you can override that in your class.

This comment has been minimized.

@mirelachirica

mirelachirica Jun 5, 2018

Contributor

What about introducing new protected virtual method for "user authentication" only and override that one?

This comment has been minimized.

@jarvte

jarvte Jun 6, 2018

Contributor

I would like @mirelachirica approach, clear method for authentication. Does not need to be in API level.

This comment has been minimized.

@ytsuboi

ytsuboi Jun 6, 2018

Contributor

I can solve this issue with @AnttiKauppila approach soon, but @mirelachirica approach is too heavy for me. ( I don't want to change structures of shared/common code, since it could affect others platform. ) In addition, I don't want to support separating a function from AT command to response.

So, can I change set_context_to_be_activated() to protected and virtual method? @mirelachirica @jarvte

This comment has been minimized.

@mirelachirica

mirelachirica Jun 6, 2018

Contributor

I don't see any risk in changing the shared code something like this:

nsapi_error_t AT_CellularNetwork::set_context_to_be_activated()
{
    // try to find or create context with suitable stack
    if (!get_context()) {
        return NSAPI_ERROR_NO_CONNECTION;
    }

    // if user has defined user name and password we need to call CGAUTH before activating or modifying context
    nsapi_error_t ret_val = do_user_authentication();
    
    return ret_val;
}

nsapi_error_t AT_CellularNetwork::do_user_authentication()
{
    if (_pwd && _uname) {
        _at.cmd_start("AT+CGAUTH=");
        _at.write_int(_cid);
        _at.write_int(_authentication_type);
        _at.write_string(_uname);
        _at.write_string(_pwd);
        _at.cmd_stop();
        _at.resp_start();
        _at.resp_stop();
        if (_at.get_last_error() != NSAPI_ERROR_OK) {
            return NSAPI_ERROR_AUTH_FAILURE;
        }
    }
    return _at.get_last_error();
}

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 6, 2018

Contributor

But with this approach, what is the need of set_context_to_be_activated function?
Also calling inheritable function from private method is not a good design.
I think the best would be to rename set_context_to_be_activated() to do_user_authentication()
We can do this without breaking anything as it was a private method.
Also is get_context() check really needed in this function or should it be in a place where this function is called (activate_context)? Usually private functions are only called when the internal status is ok. And checking what activate_context does it makes perfect sense to move this check there.

This comment has been minimized.

@mirelachirica

mirelachirica Jun 7, 2018

Contributor

I agree with @AnttiKauppila proposals. One note still: not only rename set_context_to_be_activated() to do_user_authentication() but make it also protected virtual.

And the usage like:

nsapi_error_t AT_CellularNetwork::do_user_authentication()
{
    if (_pwd && _uname) {
        _at.cmd_start("AT+CGAUTH=");
        _at.write_int(_cid);
        _at.write_int(_authentication_type);
        _at.write_string(_uname);
        _at.write_string(_pwd);
        _at.cmd_stop();
        _at.resp_start();
        _at.resp_stop();
        if (_at.get_last_error() != NSAPI_ERROR_OK) {
            return NSAPI_ERROR_AUTH_FAILURE;
        }
    }

    return NSAPI_ERROR_OK;
}

nsapi_error_t AT_CellularNetwork::activate_context()
{
    _at.lock();

    nsapi_error_t err = NSAPI_ERROR_OK;

    // try to find or create context with suitable stack
    if(get_context()) {
        // try to authenticate user before activating or modifying context
        err = do_user_authentication();
    } else {
        err = NSAPI_ERROR_NO_CONNECTION;
    }

   if (err != NSAPI_ERROR_OK) {
.....

This comment has been minimized.

@ytsuboi

ytsuboi Jun 8, 2018

Contributor

@mirelachirica @AnttiKauppila I also needed to change accessibility of get_context(). Applied suggested changes.

@AnttiKauppila

Please modify AT_CellularNetwork class as I suggested in my comment

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jun 6, 2018

[Wio 3G] Changed suggested points
Changed accessibility cellular features member functions, fixed minor target issues
@@ -420,7 +429,7 @@ void AT_CellularNetwork::ppp_status_cb(nsapi_event_t event, intptr_t parameter)
}
#endif
nsapi_error_t AT_CellularNetwork::set_context_to_be_activated()
nsapi_error_t AT_CellularNetwork::do_user_authentication()
{
// try to find or create context with suitable stack
if (!get_context()) {

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 9, 2018

Contributor

This check is not needed anymore, it is done in activate_context already

This comment has been minimized.

@ytsuboi

ytsuboi Jun 11, 2018

Contributor

Done

@@ -180,6 +178,8 @@ class AT_CellularNetwork : public CellularNetwork, public AT_CellularBase
AuthenticationType _authentication_type;
int _cell_id;
nsapi_connection_status_t _connect_status;
virtual nsapi_error_t do_user_authentication();

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 9, 2018

Contributor

No need for error value anymore

This comment has been minimized.

@mirelachirica

mirelachirica Jun 11, 2018

Contributor

Do you mean no error return value? What about NSAPI_ERROR_AUTH_FAILURE?

This comment has been minimized.

@ytsuboi

ytsuboi Jun 11, 2018

Contributor

I think this topic is not about adding target, so I didn’t make any change about this. Please avoid this topic in this PR.

nsapi_error_t QUECTEL_UG96_CellularNetwork::do_user_authentication()
{
// try to find or create context with suitable stack
if (!get_context()) {

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 9, 2018

Contributor

This check is not needed anymore, it is done in activate_context already

This comment has been minimized.

@ytsuboi

ytsuboi Jun 11, 2018

Contributor

Done

@AnttiKauppila

do_user_authentication does not need return value anymore, please remove that

@@ -180,6 +178,8 @@ class AT_CellularNetwork : public CellularNetwork, public AT_CellularBase
AuthenticationType _authentication_type;
int _cell_id;
nsapi_connection_status_t _connect_status;
virtual nsapi_error_t do_user_authentication();
bool get_context();

This comment has been minimized.

@AnttiKauppila

AnttiKauppila Jun 9, 2018

Contributor

I don't see a reason why you changed the visibility of this method. Could you explain?

This comment has been minimized.

@ytsuboi

ytsuboi Jun 10, 2018

Contributor

get_context() was called from do_user_authentication(), so there were compile error. This is why I made change.

[Error] AT_CellularNetwork.h@157,10: 'bool mbed::AT_CellularNetwork::get_context()' is private
[Error] QUECTEL_UG96_CellularNetwork.cpp@49,10: within this context
[Error] AT_CellularNetwork.h@157,10: 'bool mbed::AT_CellularNetwork::get_context()' is private
[Error] QUECTEL_UG96_CellularNetwork.cpp@49,22: within this context
[ERROR] In file included from ./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.h:21:0,
                 from ./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.cpp:18:
./mbed-os/features/cellular/framework/AT/AT_CellularNetwork.h: In member function 'virtual nsapi_error_t mbed::QUECTEL_UG96_CellularNetwork::do_user_authentication()':
./mbed-os/features/cellular/framework/AT/AT_CellularNetwork.h:157:10: error: 'bool mbed::AT_CellularNetwork::get_context()' is private
     bool get_context();
          ^
./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.cpp:49:10: error: within this context
     if (!get_context()) {
          ^
In file included from ./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.h:21:0,
                 from ./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.cpp:18:
./mbed-os/features/cellular/framework/AT/AT_CellularNetwork.h:157:10: error: 'bool mbed::AT_CellularNetwork::get_context()' is private
     bool get_context();
          ^
./mbed-os/features/cellular/framework/targets/QUECTEL/UG96/QUECTEL_UG96_CellularNetwork.cpp:49:22: error: within this context
     if (!get_context()) {
                      ^

So I'm guessing I can return get_context() to private, if I remove this check.

This comment has been minimized.

@ytsuboi

ytsuboi Jun 11, 2018

Contributor

Now, get_context() backed to private.

[Wio 3G] Changed suggested things
Removed context check from do_user_authentication.
@AnttiKauppila

This comment has been minimized.

Contributor

AnttiKauppila commented Jun 11, 2018

0xc0170 This is fine from Cellular team perspective, ready to be tested

@cmonr cmonr added needs: CI and removed needs: work labels Jun 11, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 12, 2018

/morph build

@cmonr

cmonr approved these changes Jun 12, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 13, 2018

Failed test does not appear to be caused by the PR, and mirrors an issue we observed earlier today.
/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Jun 14, 2018

@cmonr cmonr merged commit 276588f into ARMmbed:master Jun 14, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 920 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9558 cycles (+729 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment