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

Support for Twilio SIMs by default in system firmware [ch1537] #1311

Merged

Conversation

@technobly
Copy link
Member

commented Apr 26, 2017

Supersedes PR #1288 so we can release in 0.6.2-rc.2 instead of 0.7.0-rc.1.


Feature

  • [PR#1311] [Implements CH1537] [Electron] Added support for Twilio SIMs by default in system firmware.
technobly added 5 commits Apr 3, 2017
adds support for Twilio SIMs by default in system firmware
 Conflicts:
	system/src/system_network_internal.h
	system/src/system_task.cpp
flush on-boot URCs, and improve checks for "+CPIN: READY" URCs
- While testing Twilio SIM cards, power cycling frequently was part of the testing and during this it was observed that sometimes the URCs were coming after the RESP_OK on all commands following AT+CPIN?. This caused the AT+CIMI command to fail to capture the IMSI number and the system would not be configured for the Twilio SIM properly.  The issue was that new on-boot URCs were skewing the responses, and also a pattern in the retry scheme for SIM_READY was noticed that was removed.  Now while we wait to retry for SIM_READY, we also scan for "+CPIN: READY" URCs.  Also we flush URCs on boot for up to 200ms.

@technobly technobly added this to the 0.6.2 milestone Apr 26, 2017

@technobly technobly requested a review from m-mcgowan Apr 26, 2017


typedef enum { CELLULAR_NETPROV_TELEFONICA = 0,
CELLULAR_NETPROV_TWILIO = 1,
CELLULAR_NETPROV_UNKNOWN = 999 } CellularNetProv;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 26, 2017

Contributor

why 999? why not make it the largest value possible?, e.g. 0x7FFF, or 0x7FFFFFFF if you want to force the enum to 32-bits.

This comment has been minimized.

Copy link
@technobly

technobly Apr 26, 2017

Author Member

1000 providers seemed reasonable, but I'll change it to UINT_MAX.

This comment has been minimized.

Copy link
@technobly

technobly Apr 26, 2017

Author Member

I ended up changing this to a CELLULAR_NETPROV_MAX enum, to be used only in the future if we need to cycle through providers. Right now we test and set one of two.

return CELLULAR_NETPROV_TWILIO;
}
else {
return CELLULAR_NETPROV_TELEFONICA;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 26, 2017

Contributor

do we have a imsi range for telefonica? would be good to tighten up this code for future forward compatibility.

This comment has been minimized.

Copy link
@technobly

technobly Apr 26, 2017

Author Member

We do not, but that's something we can work on.

@@ -285,6 +286,16 @@ void establish_cloud_connection()
return;
}

#if PLATFORM_ID==PLATFORM_ELECTRON_PRODUCTION

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 26, 2017

Contributor

rather than hard-coding the provider port, keep-alive, APN and other details. it would be more flexible and less code to create structs in the HAL that describe these things as data, keyed on the provider ID, so that we don't have any conditional logic for each provider.

This comment has been minimized.

Copy link
@technobly

technobly Apr 26, 2017

Author Member

I started there, but opted out because it seemed like more complexity at the time. I'll look at refactoring it.

@technobly technobly added feature and removed enhancement labels Apr 26, 2017

@technobly technobly merged commit f31c294 into release/v0.6.2-rc.2 Apr 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/electron/twilio-apn-support-062rc2 branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.