Cellular: added setting of data carrier support for UART. #8971

jarvte · 2018-12-05T07:52:46Z

Description

In case when UART is used to communicate to modem we should use data carrier detect to detect disconnect faster when in PPP mode. This PR fixes that missing feature,

@AriParkkila @mirelachirica please review

Pull request type

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

kjbracey

Feels a bit weird putting Serial-specific knowledge into this generic class, but okay with it as long as it's done right and still works for other FileHandle types. So some notes on that.

kjbracey · 2018-12-05T09:31:12Z

features/cellular/framework/AT/AT_CellularContext.cpp

@@ -614,6 +644,22 @@ void AT_CellularContext::ppp_status_cb(nsapi_event_t ev, intptr_t ptr)
    _device->cellular_callback(ev, ptr);
 }

+void AT_CellularContext::ppp_disconnected()
+{
+#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN)


I don't think this is conditional on that - there are other possible implementations of enable_hup. Could certainly work over USB serial.

removed #ifdef

kjbracey · 2018-12-05T09:32:37Z

features/cellular/framework/AT/AT_CellularContext.cpp

+void AT_CellularContext::enable_hup(bool enable)
+{
+    if (_dcd_pin != NC) {
+        (static_cast<UARTSerial *>(_fh))->set_data_carrier_detect(enable ? _dcd_pin : NC, _active_high);


Don't need the extra () - the static_cast has its own built-in which seem clear enough to me.

kjbracey · 2018-12-05T09:38:12Z

features/cellular/framework/API/CellularContext.h

@@ -230,6 +239,17 @@ class CellularContext : public CellularBase {
    */
    virtual void cellular_callback(nsapi_event_t ev, intptr_t ptr) = 0;

+#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN && NSAPI_PPP_AVAILABLE)


Not sure whether enable_hup really needs to be conditional on PPP, but it certainly isn't conditional on UARTSerial, as an abstract API. I'd simplify by making this unconditional.

If it is conditional in any way, you should at least provide a dummy method in the #else so callers don't all need ifdef.

(Callers definitely shouldn't have ifdef, because maybe someone has inherited from AT_CellularDevice and provided an alternate enable_hup for a totally alien FileHandle).

removed #ifdef

kjbracey · 2018-12-05T09:41:21Z

features/cellular/framework/API/CellularDevice.h

@@ -70,6 +70,22 @@ class CellularDevice {
     *
     */
    virtual CellularContext *create_context(FileHandle *fh = NULL, const char *apn = NULL) = 0;
+#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN && NSAPI_PPP_AVAILABLE)


This does need to be conditional on serial+interrupt_in, because it takes UARTSerial, but again doesn't necessarily need to be conditional on PPP. Wouldn't do any harm for an app to call this one, if it knew it was using UARTSerial, but wasn't committed about PPP or not.

Why are the apn and serial parameters reversed? Overload issue?

removed #ifdef. _Yes overload when calling internally to avoid copy-paste.

kjbracey · 2018-12-05T09:44:07Z

features/cellular/framework/AT/AT_CellularContext.cpp

@@ -592,18 +613,27 @@ nsapi_error_t AT_CellularContext::open_data_channel()
    }

    _at.set_is_filehandle_usable(false);
-
+#if (DEVICE_SERIAL && DEVICE_INTERRUPTIN)


Mustn't be conditional - see other comments.

removed #ifdef

kjbracey · 2018-12-10T10:08:02Z

features/cellular/framework/AT/AT_CellularDevice.cpp

+CellularContext *AT_CellularDevice::create_context(const char *const apn, UARTSerial *serial, PinName dcd_pin,
+                                                   bool active_high)
+{
+    AT_CellularContext *ctx = (AT_CellularContext *)create_context(serial, apn);


If there was an overload problem on this call, then rather than reverse the parameters, you could do

// Call FileHandle base version - explict upcast to avoid recursing into ourselves create_context(static_cast<FileHandle *>(serial), apn);

Bit ugly, but I'd rather have an ugly internal than a weird inconsistent public API.

I note that create_context should really return AT_CellularContext -when overriding, a derived class is allowed to return a more-specific derived pointer.

Nice, that's a better way, did not think of that.

kjbracey · 2018-12-10T10:13:26Z

features/cellular/framework/AT/AT_CellularContext.cpp

+    _active_high = active_high;
+    _fh = serial;
+    _at.set_file_handle(_fh);
+    enable_hup(false);


I'd be inclined to call set_file_handle(static_cast<FileHandle *>(serial)) here too for consistency of logic with the create_context, even though it doesn't really save anything.

And hey, if you do that explicit upcast in two places it becomes a recognisable pattern!

No, I meant call AT_CellularContext::set_file_handle(FileHandle *)

Do'h, I need to sleep more :) Fixed.

kjbracey · 2018-12-10T10:17:25Z

features/cellular/framework/AT/AT_CellularContext.cpp

+    // after ppp disconnect if we wan't to use same at handler we need to set filehandle again to athandler so it
+    // will set the correct sigio and nonblocking
+    _at.lock();
+    _at.set_file_handle(_at.get_file_handle());


This line seems like daft magic. Surely set_is_filehandle_usable(true) should know it needs to reset settings itself?

true, fixed.

kjbracey · 2018-12-10T13:03:13Z

features/cellular/framework/API/CellularDevice.h

+     *  @return         new instance of class CellularContext or NULL in case of failure
+     *
+     */
+    virtual CellularContext *create_context(UARTSerial *serial, const char *apn, PinName dcd_pin = NC,


This and the other declaration should also still have the = NULL default on apn I assume?

Yep, added.

cmonr · 2018-12-12T20:51:17Z

@kjbracey-arm Mind doing a quick re-review?

@ARMmbed/mbed-os-wan Thoughts?

cmonr · 2018-12-17T20:36:39Z

CI started

mbed-ci · 2018-12-17T22:38:59Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR

jarvte · 2018-12-18T06:12:53Z

Fixed ci build

0xc0170 · 2018-12-18T10:51:57Z

CI restarted (might take a while as the queue is filling up)

mbed-ci · 2018-12-18T10:57:56Z

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

0xc0170 · 2018-12-18T11:04:19Z

Unittest failed, related to the changes, please review

jarvte · 2018-12-18T12:51:41Z

Unittest failed, related to the changes, please review

Fixed.

0xc0170 · 2018-12-18T12:59:54Z

CI restarted

mbed-ci · 2018-12-18T20:47:45Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

0xc0170 requested review from kjbracey and a team December 5, 2018 08:54

0xc0170 added the needs: review label Dec 5, 2018

kjbracey suggested changes Dec 5, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Dec 5, 2018

jarvte force-pushed the add_datacarrier_detect branch 2 times, most recently from 9a946aa to 55246e3 Compare December 10, 2018 08:15

kjbracey suggested changes Dec 10, 2018

View reviewed changes

jarvte force-pushed the add_datacarrier_detect branch from 55246e3 to 587cc23 Compare December 10, 2018 12:17

kjbracey reviewed Dec 10, 2018

View reviewed changes

jarvte force-pushed the add_datacarrier_detect branch from 587cc23 to 7606d8a Compare December 11, 2018 12:57

cmonr added the release-version: feature branch label Dec 12, 2018

cmonr added needs: review and removed needs: work labels Dec 12, 2018

kjbracey approved these changes Dec 14, 2018

View reviewed changes

AriParkkila approved these changes Dec 17, 2018

View reviewed changes

AnttiKauppila approved these changes Dec 17, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Dec 17, 2018

cmonr added ready for merge needs: CI and removed needs: CI ready for merge labels Dec 17, 2018

cmonr added needs: work and removed needs: CI labels Dec 18, 2018

jarvte force-pushed the add_datacarrier_detect branch from 7606d8a to fa18876 Compare December 18, 2018 05:58

Cellular: added setting of data carrier support for UART.

e4956ce

jarvte force-pushed the add_datacarrier_detect branch from fa18876 to e4956ce Compare December 18, 2018 12:51

cmonr added ready for merge and removed needs: work labels Dec 19, 2018

0xc0170 merged commit a7f4388 into ARMmbed:feature-cellular-refactor Dec 19, 2018

jarvte deleted the add_datacarrier_detect branch December 19, 2018 10:24

cmonr removed the ready for merge label Dec 20, 2018

Cellular: added setting of data carrier support for UART. #8971

Cellular: added setting of data carrier support for UART. #8971

Uh oh!

Conversation

jarvte commented Dec 5, 2018

Description

Pull request type

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Dec 12, 2018

Uh oh!

cmonr commented Dec 17, 2018

Uh oh!

mbed-ci commented Dec 17, 2018

Uh oh!

jarvte commented Dec 18, 2018

Uh oh!

0xc0170 commented Dec 18, 2018

Uh oh!

mbed-ci commented Dec 18, 2018

Uh oh!

0xc0170 commented Dec 18, 2018

Uh oh!

jarvte commented Dec 18, 2018

Uh oh!

0xc0170 commented Dec 18, 2018

Uh oh!

mbed-ci commented Dec 18, 2018

Uh oh!

Uh oh!