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: AT handler API #8749

Merged
merged 2 commits into from Dec 20, 2018

Conversation

Projects
None yet
6 participants
@TeemuKultala
Copy link
Contributor

commented Nov 15, 2018

Description

CellularContext get_at_handler() API added, which can be used for sending additional AT commands to a modem for debugging etc. purposes. Tested with unit tests.

Pull request type

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

@AriParkkila please review

@AriParkkila
Copy link
Contributor

left a comment

I guess this is not really a breaking change.

features/cellular/framework/API/CellularContext.h Outdated
* the modem for debugging etc. purposes
* @return Reference to the ATHandler in use
*/
virtual ATHandler &get_at_handler() = 0;

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Nov 15, 2018

Contributor

CellularContext is not always derived from AT_CellularContext. Any way to use here more generic interface than ATHandler?

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Nov 15, 2018

Contributor

It's not now clear that handler reference may die when context is deleted. Should probably use reference counting similar to other interfaces?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Nov 15, 2018

Contributor

It might make sense for this thing to return a pointer, so it can return NULL if the implementation doesn't have an ATHandler available. It should only be a reference if it is a requirement that an ATHandler is available in every CellularContext.

(For comparison, NetworkInterface::emacInterface returns a pointer, as a NetworkInterface obviously doesn't have to be an EMACInterface. But EMACInterface::get_emac() returns a reference, as an EMACInterface must have an EMAC.)

If you're returning an ATHandler you were given and you're using for your context, then presumably CellularContext has no lifetime view on it. Reference counting would be down to ATHandler itself - if it has a reference count, the CellularContext should increment it before passing it on. Or you could use mbed::SharedPtr. Not sure how well that works - never used it myself. Or just wait for C++11 std::shared_ptr. Coming real soon, maybe.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@TeemuKultala Fyi, this needs a rebase

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Is this targeting 5.11 ? There's been rebase needed and reviewers !

@TeemuKultala

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

@0xc0170 The target for this is 5.12

features/cellular/framework/AT/ATHandler.cpp Outdated
@@ -110,6 +110,8 @@ ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, int timeout, const char

set_file_handle(fh);
}
ATHandler *ATHandler::_atHandlers = NULL;
PlatformMutex ATHandler::_getReleaseMutex;

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

This needs to be in a SingletonPtr to avoid static memory wastage.

features/cellular/framework/AT/ATHandler.cpp Outdated
_getReleaseMutex.lock();
ATHandler *atHandler = _atHandlers;
while (atHandler) {
if (atHandler->get_file_handle() == fileHandle) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

If you end up sharing an ATHandler, what's supposed to happen if the queue, timeout, delimiter or send_delay parameters mismatch? If you can't cope with a mismatch, an error or assert might be in order.

This comment has been minimized.

Copy link
@TeemuKultala

TeemuKultala Dec 4, 2018

Author Contributor

I think it is correct to use the existing parameters, if there is a mismatch in the new ones. Also queue etc. are not currently available through a public API for check. @kjbracey-arm should an API be created for this?

features/cellular/framework/AT/ATHandler.cpp Outdated
@@ -160,6 +162,69 @@ void ATHandler::set_is_filehandle_usable(bool usable)
_is_fh_usable = usable;
}


// each parser is associated with one filehandle (that is UART)
ATHandler *ATHandler::get(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

For consistency with all similar APIs, I'd prefer get_instance. (We mostly have get_default_instance, but there are 1 or 2 other get_instances for non-default things like this).

features/cellular/framework/AT/ATHandler.cpp Outdated
return NULL;
}

_getReleaseMutex.lock();

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

Consider ScopedLock<PlatformMutex> to avoid having to worry about the manual unlocks. If the rest of your code is doing it all manually, maybe change everything later.

features/cellular/framework/AT/ATHandler.cpp Outdated
return atHandler;
}

nsapi_error_t ATHandler::release(ATHandler *at_handler)

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

This should be a non-static method on the ATHandler. (Or at least it should be exposed like that, even if internally you have a private _release for the factory).

I'd slightly prefer it to be close(), which tallies up with a couple of other APIs where the close() is expected to do the deletion if it was allocated by a factory. (eg Socket and FileHandle).

UNITTESTS/stubs/ATHandler_stub.cpp Outdated
}

void ATHandler::set_file_handle(FileHandle *fh)
{
}

// each parser is associated with one filehandle (that is UART)

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

Do you really need to paste two complete working routines into the stubs?

If you do, I suggest separating these two factory functions into a standalone ATHandler_factory.cpp, and including that working file in the tests, not copy-pasting the code.

*
* @param fileHandle filehandle used for reading AT responses and writing AT commands
* @param queue Event queue used to transfer sigio events to this thread
* @param timeout Timeout when reading for AT response

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

Timeout type mismatches the constructor.

return atHandler;
ATHandler *AT_CellularDevice::get_at_handler()
{
return get_at_handler(NULL);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 3, 2018

Contributor

What does this do?

This comment has been minimized.

Copy link
@TeemuKultala

TeemuKultala Dec 4, 2018

Author Contributor

@kjbracey-arm Through this an ATHandler instance is available for use outside CellularDevice

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 5, 2018

Contributor

Ah, was just confused at what passing NULL was doing. See it now.

features/cellular/framework/AT/ATHandler_factory.cpp Outdated
return NULL;
}

_getReleaseMutex->lock();

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 5, 2018

Contributor

Just been working on some similar "get_instance" code, and I'm wondering if having your own mutex is overkill, in terms of RAM usage, which is increased by the need for the SingletonPtr wrapper. This is not a frequently-used path.

So maybe just use singleton_lock() and singleton_unlock(). That lets you share the system-wide mutex with the core SingletonPtr and static C++ objects - no RAM cost, and no real downside for init/shutdown-type calls like this.

return atHandler;
ATHandler *AT_CellularDevice::get_at_handler()
{
return get_at_handler(NULL);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 5, 2018

Contributor

Ah, was just confused at what passing NULL was doing. See it now.

features/cellular/framework/AT/ATHandler.h Outdated
@@ -83,6 +84,26 @@ class ATHandler {
*/
FileHandle *get_file_handle();

/** Get a new ATHandler instance, and update the linked list

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 5, 2018

Contributor

Need to document how you match existing instances (same FileHandle), and what happens with the other parameters when you do.

Also need to document that because the produced handlers are reference counted, close() must be called when finishing with the instance.

features/cellular/framework/AT/ATHandler.h Outdated
static ATHandler *get_instance(FileHandle *fileHandle, events::EventQueue &queue, uint32_t timeout,
const char *delimiter, uint16_t send_delay, bool debug_on);

/** Close the current ATHandler instance, if the reference count to it is 0

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 5, 2018

Contributor

This needs to be documented that it must only be called on ATHandlers obtained from get_instance - it would break for other ATHandlers at present.

(Although it could in principle have adaptive behaviour - eg be a no-op by default if constructed otherwise, and do the list stuff if a "factory-allocate" flag is set, which get_instance would).

features/cellular/framework/AT/AT_CellularDevice.cpp Outdated
while (atHandler) {
atHandler->set_at_timeout(_default_timeout, true); // set as default timeout
atHandler = atHandler->_nextATHandler;
if (_network) {

This comment has been minimized.

Copy link
@AriParkkila

AriParkkila Dec 13, 2018

Contributor

This could be iterator over ATHandlers.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@kjbracey-arm Can you rereview?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

CI started in the meantime

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 18, 2018

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Dec 18, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@TeemuKultala Question about this PR.

Would it be possible for this to instead go into the feature-cellular-refactor branch?

@cmonr cmonr added needs: review and removed ready for merge labels Dec 18, 2018

@TeemuKultala TeemuKultala changed the base branch from master to feature-cellular-refactor Dec 20, 2018

@TeemuKultala TeemuKultala changed the base branch from feature-cellular-refactor to master Dec 20, 2018

@TeemuKultala

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@cmonr Looks like changing base to feature-cellular-refactor includes over 200 commits into this PR. So why is it not possible merge into master?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@cmonr Looks like changing base to feature-cellular-refactor includes over 200 commits into this PR. So why is it not possible merge into master?

We'll bringthis into master. The problem we were hoping to avoid was the issue where if future cellular changes are put on top of this change, they'll also end up being directed to 5.12.

@cmonr cmonr added ready for merge and removed needs: review labels Dec 20, 2018

@0xc0170 0xc0170 merged commit 671c061 into ARMmbed:master Dec 20, 2018

21 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-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 0 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9877 cycles (+365 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 8372B (+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

@TeemuKultala TeemuKultala deleted the TeemuKultala:cellular_at_handler_api branch Mar 7, 2019

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.