Cellular: Mark to be deprecated in CellularDevice #11935

AriParkkila · 2019-11-25T07:22:14Z

Description

Summary of change

Mark to be deprecated functions in CellularDevice.

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@AnttiKauppila

Release Notes

Summary of changes

Impact of changes

Migration actions required

0xc0170 · 2019-11-25T07:39:06Z

CI started

kjbracey · 2019-11-25T08:41:49Z

features/cellular/framework/API/CellularDevice.h

    FileHandle &get_file_handle() const;

+    /** Get the current filehandle.
+     *
+     *  @return pointer to filehandle or NULL


When will this return null? As I recall part of the reason for original API was that it wouldn't.

Filehandle is used to identify serial link and/or data channels. When CellularDevice is over a non-filehandle alike link, it's possible that filehandle is NULL since data channels may not be immediately available.

Sounds like some sort of redesign being planned? Got any more info?

Deprecations usually accompany or follow redesigns, not precede them - it's a bit hard to review a deprecation that's a hint of "things to come".

At the minute, the FileHandle is part of the fundamental channel ownership. Are you planning to increase complexity here by letting lower levels kill themselves/not exist while the higher levels remain active?

At the minute, we're generally preferring the "suspend/resume" API for that sort of thing, as that allows you to retain ownership.

Marked CellularDevice::get_file_handle as deprecated.

@kjbracey-arm the update resolved this?

mbed-ci · 2019-11-25T10:52:38Z

Test run: SUCCESS

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

0xc0170 · 2019-11-25T12:01:39Z

CI restarted

mbed-ci · 2019-11-25T14:44:53Z

Test run: SUCCESS

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

kjbracey · 2019-11-26T09:29:48Z

features/cellular/framework/API/CellularDevice.h

    void stop();

    /** Get the current FileHandle item used when communicating with the modem.
     *
     *  @return reference to FileHandle
     */
+    MBED_DEPRECATED_SINCE("mbed-os-5.15", "Use CellularDevice::get_at_handler()->get_filehandle() instead.")


Still not quite following - the CellularDevice is tied to a particular file handle. Always (at present). It might be switching control of that file handle between different users, in particular the AT handler and the PPP driver.

How does asking for the handle from the AT handler's copy improve things?

Is there some change of CellularDevice planned, so that it (and hence the AT handler) change handle? I can see why ATHandler was designed to be a bit more flexible on file handle switching - it's not always in control of the file handle, cos it has to hand over to PPP, but not sure that extends to CellularDevice.

Also, that suggested call seems unsafe as get_at_handler is a reference-counted claim - needs to be balanced with a release_at_handler().

@kjbracey-arm FileHandle may have users such AT, PPP, MUX, and also application need to use it to enable deep-sleep. However, CellularDevice has no use for FileHandle per se, and thus, it kind of makes sense to ask that from CellularDevice dynamically, like if (CellularDevice::get_at_handler()) { get_filehandle()->do_something(); }.

This change is not at all about changing AT based cellular implementation. But there are other adaptations, where CellularDevice need not to have FileHandle, because a modem adaptation has separate "link" channels (also possibly in shared memory) for control and user data and also there can be many data channels, for example Android RIL adaptations may be deferring creation of data channels, and if FileHandle would be used in original (vendor specific which is hard to force) adaptation, it could still be NULL initially and not good as a reference.

A call to get_at_handler() should be balanced with a release, which is documented in CellularDevice::get_at_handler().

A call to get_at_handler() should be balanced with a release, which is documented in CellularDevice::get_at_handler().

That's not possible in either the example in the deprecation, or your if (get_at_handler()) code above, because you're not storing the return value to pass to release. So you're suggesting "could-never-be-correct" code snippets which people are very likely to copy.

A FileHandle is to a large extent a "handle" to a system, and there's no reason why a more-complex-than-just-a-single-stream device couldn't still present as something derived from a FileHandle. That would allow common functionality like the enable_input/enable_output calls.

Still, that's more Linux than Mbed OS, so okay, yes, if the base CellularDevice itself doesn't need a FileHandle, then it could be adjusted to allow for null, in case the derived class doesn't need it either. If that's the plan, then I suggest you make a more complete patch doing that. Something like:

Remove the MBED_ASSERT(fh) from CellularDevice constructor, so you can actually pass null.

Add a = nullptr to the declaration so you can default-construct CellularDevice with no filehandle

Deprecate the get_file_handle in favour of a get_file_handle_ptr so you can read null.

I still don't see any need to complicate accessing the file handle - as long as you're passing it to CellularDevice's constructor, and it's having to store it anyway to pass to open_network it seems totally reasonable to be able to read it back out from the CellularDevice.

Maybe you could have no read in the base class, which would open the door the the base not storing it, but you don't yet have an adequate replacement - if it was in AT_CellularDevice only that's not public, and neither is ATHandler.

Has this been resolved ? IF not ,can it be asap?

40Grit · 2019-11-28T05:18:33Z

@kjbracey-arm it would be nice if there was a very clean and standard way to organize, serialize and control access to hardware resources both in mcu and on the board.

Something about file handles has always seemed like a stretched concept to me.

@AGlass0fMilk

kjbracey · 2019-11-28T12:58:02Z

@kjbracey-arm it would be nice if there was a very clean and standard way to organize, serialize and control access to hardware resources both in mcu and on the board.

Something about file handles has always seemed like a stretched concept to me.

The FileHandle stuff isn't really used for that, and it would be a stretch here. But it is being used to abstract out the data stream concept. The original intent here was to not care about serial versus some SPI channel versus USB for the AT handler or PPP. And enabling/disabling input on a stream is a reasonable generic concept for a stream.

We are intending to extend a suspend/resume pattern across a bunch of classes to aid with power saving, but beyond that any more serious "organisation" and "control" structure would often lead to quite a lot of RAM usage. We can't really afford the RAM structures to construct a lovely device tree hierarchy. And it would be RAM, not ROM, because we do fully permit dynamic device creation.

(We're making some moves to allow things to be static/compile-time where possible though - look at the pinmap PR for an example of that.)

features/cellular/framework/API/CellularDevice.h

0xc0170 · 2019-11-29T10:39:17Z

@AnttiKauppila @AriParkkila might be OoO today, can you help here?

0xc0170 · 2019-11-29T12:27:56Z

CI started

mbed-ci · 2019-11-29T14:23:39Z

Test run: SUCCESS

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

0xc0170 added the release-version: 5.15.0-rc1 label Nov 25, 2019

0xc0170 approved these changes Nov 25, 2019

View reviewed changes

0xc0170 added the needs: CI label Nov 25, 2019

AnttiKauppila approved these changes Nov 25, 2019

View reviewed changes

kjbracey reviewed Nov 25, 2019

View reviewed changes

0xc0170 added needs: review and removed needs: CI labels Nov 25, 2019

AriParkkila force-pushed the cell-depr-device branch from 83ab23f to d1456cd Compare November 25, 2019 10:32

AnttiKauppila approved these changes Nov 25, 2019

View reviewed changes

0xc0170 requested a review from kjbracey November 25, 2019 17:29

kjbracey reviewed Nov 26, 2019

View reviewed changes

0xc0170 reviewed Nov 28, 2019

View reviewed changes

features/cellular/framework/API/CellularDevice.h Show resolved Hide resolved

Cellular: Mark CellularDevice::stop deprecated

088140e

AriParkkila force-pushed the cell-depr-device branch from d1456cd to 088140e Compare November 29, 2019 12:25

AnttiKauppila approved these changes Nov 29, 2019

View reviewed changes

0xc0170 approved these changes Nov 29, 2019

View reviewed changes

0xc0170 requested a review from kjbracey November 29, 2019 12:29

0xc0170 added needs: CI and removed needs: review labels Nov 29, 2019

kjbracey approved these changes Nov 29, 2019

View reviewed changes

0xc0170 removed the needs: CI label Nov 29, 2019

0xc0170 added the ready for merge label Nov 29, 2019

0xc0170 merged commit 24c6c4c into ARMmbed:master Nov 29, 2019

adbridge removed the ready for merge label Nov 29, 2019

Cellular: Mark to be deprecated in CellularDevice #11935

Cellular: Mark to be deprecated in CellularDevice #11935

Uh oh!

Conversation

AriParkkila commented Nov 25, 2019

Description

Summary of change

Documentation

Pull request type

Test results

Reviewers

Release Notes

Summary of changes

Impact of changes

Migration actions required

Uh oh!

0xc0170 commented Nov 25, 2019

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!

mbed-ci commented Nov 25, 2019

Uh oh!

0xc0170 commented Nov 25, 2019

Uh oh!

mbed-ci commented Nov 25, 2019

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!

40Grit commented Nov 28, 2019

Uh oh!

kjbracey commented Nov 28, 2019

Uh oh!

Uh oh!

0xc0170 commented Nov 29, 2019

Uh oh!

0xc0170 commented Nov 29, 2019

Uh oh!

mbed-ci commented Nov 29, 2019

Uh oh!

Uh oh!