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

Cellular: UBLOX_C030_N211 Cellular API's #9637

Merged
merged 5 commits into from Mar 14, 2019

Conversation

Projects
None yet
8 participants
@mudassar-ublox
Copy link
Contributor

commented Feb 7, 2019

Description

This PR updates the cellular API's for UBLOX target UBLOX_C030_N211. It has target type UBLOX_N2XX. These API support only UDP operations.

Supported Targets: UBLOX_C030_N211
Tool Chains: ARM, GCC_ARM, IAR

ARM_Build.txt
GCC_Build.txt
IAR_Build.txt

Pull request type

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

Reviewers

@mudassar-ublox mudassar-ublox changed the title C030_N211 cellular api Cellular: UBLOX_C030_N211 Cellular API's Feb 7, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Feb 7, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@mudassar-ublox, thank you for your changes.
@ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Will this conflict with #9568 ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@ARMmbed/mbed-os-wan Can you please review and advice how to resolve conflicts here?

Conflicting files
features/cellular/TESTS/api/cellular_power/main.cpp
features/cellular/framework/API/CellularPower.h
features/cellular/framework/AT/AT_CellularBase.h
features/cellular/framework/AT/AT_CellularPower.cpp
features/cellular/framework/AT/AT_CellularPower.h
features/cellular/framework/AT/AT_CellularSMS.cpp

@fahim-ublox

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Pull request is getting updated but any advice to resolve conflicts will be better. Thanks

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@fahim-ublox To resolve the conflicts, this PR needs to be rebased against master.
The rebase process will pause for each conflict to allow for manual resolution.

It's possible that @ARMmbed/mbed-os-wan might need to help.

@AriParkkila
Copy link
Contributor

left a comment

Unfortunately feature-cellular-refactor introduced quite many (breaking) changes that need to be resolved here, but the changes seem to be quite straight forward. Note that after SupportedFeature was changed to CellularProperty it requires that every modem driver must have a complete property map and thus need to be updated when adding new properties like AT_CellularSMSMmodeText.

features/cellular/TESTS/api/cellular_power/main.cpp Outdated
@@ -81,7 +81,7 @@ static void test_power_interface()
TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED);
wait_for_power(pwr);

TEST_ASSERT(pwr->set_power_level(1, 0) == NSAPI_ERROR_OK);
TEST_ASSERT(pwr->set_power_level(1, 0, MBED_CONF_APP_CELLULAR_SIM_PIN) == NSAPI_ERROR_OK);

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

Test was removed.

features/cellular/framework/API/CellularPower.h Outdated
@@ -93,7 +93,7 @@ class CellularPower {
* @return NSAPI_ERROR_OK on success
* NSAPI_ERROR_DEVICE_ERROR on failure
*/
virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0) = 0;
virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0, const char *sim_pin = NULL) = 0;

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

CellularPower API was removed.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp Outdated
{
}

nsapi_error_t UBLOX_N2XX_CellularPower::on()

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

on() was split to soft/hard_power_on

hard_power_on to call ::onboard_modem_init()
hard_power_off to call ::onboard_modem_deinit()
soft_power_on to call ::onboard_modem_power_up()
soft_power_off to call ::onboard_modem_power_down()

features/cellular/framework/AT/AT_CellularPower.cpp Outdated
@@ -56,7 +56,7 @@ nsapi_error_t AT_CellularPower::set_at_mode()
return _at.unlock_return_error();
}

nsapi_error_t AT_CellularPower::set_power_level(int func_level, int do_reset)
nsapi_error_t AT_CellularPower::set_power_level(int func_level, int do_reset, const char *sim_pin)

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

File was removed.

features/cellular/framework/AT/AT_CellularPower.h Outdated
@@ -40,7 +40,7 @@ class AT_CellularPower : public CellularPower, public AT_CellularBase {

virtual nsapi_error_t set_at_mode();

virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0);
virtual nsapi_error_t set_power_level(int func_level, int do_reset = 0, const char *sim_pin = NULL);

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

File was removed.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.h Outdated
@@ -0,0 +1,45 @@
/*

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

CellularPower api was removed, remove this file.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp Outdated
}


nsapi_error_t UBLOX_N2XX_CellularPower::set_power_level(int func_level, int do_reset, const char *sim_pin)

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

Function was removed.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp Outdated
return NSAPI_ERROR_OK;
}

nsapi_error_t UBLOX_N2XX_CellularPower::set_at_mode()

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

Function was changed to CellularDevice::init.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularPower.cpp Outdated
@@ -0,0 +1,92 @@
/*

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

CellularPower api was removed, remove this file.

features/cellular/framework/targets/UBLOX/N2XX/UBLOX_N2XX_CellularNetwork.h Outdated
@@ -0,0 +1,43 @@
/*

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Feb 12, 2019

Contributor

This file can likely be removed.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@mudassar-ublox Do you need any assistance for updating this PR?

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@mudassar-ublox Do you need any assistance for updating this PR?

There are few other commands which are also not supported for this target. So I am adding them in CellularProperty in AT_CellularBase like this, commands CNMI, CSMP, CMGF, CSDH, is that fine

enum CellularProperty {
    PROPERTY_C_EREG,            // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_C_GREG,            // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_C_REG,             // AT_CellularNetwork::RegistrationMode. What support modem has for this registration type.
    PROPERTY_AT_CGSN_WITH_TYPE, // 0 = not supported, 1 = supported. AT+CGSN without type is likely always supported similar to AT+GSN.
    PROPERTY_AT_CGDATA,         // 0 = not supported, 1 = supported. Alternative is to support only ATD*99***<cid>#
    PROPERTY_AT_CGAUTH,         // 0 = not supported, 1 = supported. APN authentication AT commands supported
    PROPERTY_AT_CNMI,           // 0 = not supported, 1 = supported. New message (SMS) indication AT command
    PROPERTY_AT_CSMP,           // 0 = not supported, 1 = supported. Set text mode AT command
    PROPERTY_AT_CMGF,           // 0 = not supported, 1 = supported. Set preferred message format AT command
    PROPERTY_AT_CSDH,           // 0 = not supported, 1 = supported. Show text mode AT command
    PROPERTY_IPV4_PDP_TYPE,     // 0 = not supported, 1 = supported. Does modem support IPV4?
    PROPERTY_IPV6_PDP_TYPE,     // 0 = not supported, 1 = supported. Does modem support IPV6?
    PROPERTY_IPV4V6_PDP_TYPE,   // 0 = not supported, 1 = supported. Does modem support dual stack IPV4V6?
    PROPERTY_NON_IP_PDP_TYPE,   // 0 = not supported, 1 = supported. Does modem support Non-IP?
    PROPERTY_MAX
}
@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

PROPERTY_AT_CGAUTH

This is probably not needed, but you should simply override APN authentication similar to UBLOX_AT_CellularContext::activate_profile.

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

PROPERTY_AT_CGAUTH

This is probably not needed, but you should simply override APN authentication similar to UBLOX_AT_CellularContext::activate_profile.

you mean to override AT_CellularContext::do_user_authentication() function ?
PROPERTY_AT_CGAUTH already handled in it so why need to override it.

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@mudassar-ublox if the modem supports AT+CGAUTH then my comment was incorrect :)

And yes, it's fine to add new AT commands you mentioned (you just need to add those in all modem drivers).

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@mudassar-ublox if the modem supports AT+CGAUTH then my comment was incorrect :)

And yes, it's fine to add new AT commands you mentioned (you just need to add those in all modem drivers).

For other modem drivers, I have not idea whether these commands are supported for them or not. Can you please do that accordingly after my PR. For other UBLOX drivers I will update it.

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@mudassar-ublox unfortunately your PR will never pass without adding properties to each driver, but you can define them all simply supported due to that won't change any current behavior.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Removing 5.12 release label as this needs further work and review

@mudassar-ublox mudassar-ublox force-pushed the u-blox:C030_N211_Cellular_Driver branch Feb 21, 2019

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

I have updated PR, please review now.

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@mudassar-ublox cellular properties look good. but still need to refactor power and SMS classes.

@mudassar-ublox mudassar-ublox force-pushed the u-blox:C030_N211_Cellular_Driver branch Feb 22, 2019

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

I have updated PR, power classes are removed and SMS classes are already updated. Please review and let me know if anything else is required.

@cmonr cmonr added needs: review and removed needs: work labels Feb 22, 2019

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I think psock->id = index; is required, to identify sockets by its ID ?

I noticed that UBLOX_N2XX_CellularStack::create_socket_impl reads that from modem as socket->id = sock_id; and because for example AT_CellularStack::find_socket does not check created flag it will mess up 'socket index' and ' modem socket id'. You are right that fix is not good in general, but I you should be able to use it as workaround if you want to test UBLOX_N2XX.

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Yes, in Ublox api's socket->id is read from modem but AT_CellularStack is parent class and other targets are also using it, i have not mush info about that are they doing the same as we are doing. However initialization is incorrect, it need to be removed.
'AT_CellularStack::find_socket' is checking NULL socket pointer so I think find_socket will return correct index .

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@AriParkkila you shared Astyle.exe on my previous pull request. Can I use that on this PR for Astyling issuse?
#7619 (comment)

@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Updated api and removed Astyle issue. Please review now.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@mudassar-ublox There are still astyle issues. Please review.

Astyle Quickfix: Feel free to run the following script to correct this PR's astyle issues identified by Travis CI.

git apply -3 <<'PATCH' && git commit -am "Applied suggested astyle fixes" && git push
diff --git a/features/cellular/TESTS/api/cellular_sms/main.cpp b/features/cellular/TESTS/api/cellular_sms/main.cpp
index 7947696dd1..79390bc0d6 100644
--- a/features/cellular/TESTS/api/cellular_sms/main.cpp
+++ b/features/cellular/TESTS/api/cellular_sms/main.cpp
@@ -111,20 +111,20 @@ static void test_sms_initialize_pdu_mode()
 {
     nsapi_error_t err = sms->initialize(CellularSMS::CellularSMSMmodePDU);
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 static void test_set_cscs()
 {
     nsapi_error_t err = sms->set_cscs("IRA");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
     err = sms->set_cscs("UCS2");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
     err = sms->set_cscs("GSM");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 static void test_set_csca()
@@ -144,7 +144,7 @@ static void test_set_cpms_me()
 {
     nsapi_error_t err = sms->set_cpms("ME", "ME", "ME");
     TEST_ASSERT(err == NSAPI_ERROR_OK || err == NSAPI_ERROR_UNSUPPORTED || (err == NSAPI_ERROR_DEVICE_ERROR &&
-                ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
+                                                                            ((AT_CellularSMS *)sms)->get_device_error().errCode == SIM_BUSY));
 }
 
 #ifdef MBED_CONF_APP_CELLULAR_PHONE_NUMBER
PATCH
@mudassar-ublox

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@cmonr suggested patch is applied, please review now.

@cmonr cmonr requested review from AriParkkila and RobMeades and removed request for AriParkkila Mar 6, 2019

@cmonr cmonr added needs: review and removed needs: work labels Mar 6, 2019

@RobMeades
Copy link
Contributor

left a comment

LGTM.

@cmonr

cmonr approved these changes Mar 7, 2019

@0xc0170

0xc0170 approved these changes Mar 8, 2019

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Mar 8, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@AriParkkila Happy with it now?

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 11, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 12, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

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

@0xc0170 0xc0170 merged commit 76fe726 into ARMmbed:master Mar 14, 2019

28 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-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-36 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 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 10182 cycles (+193 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.