Cellular: AT handler API #8749
Cellular: AT handler API #8749
Conversation
|
@AriParkkila please review |
| * the modem for debugging etc. purposes | ||
| * @return Reference to the ATHandler in use | ||
| */ | ||
| virtual ATHandler &get_at_handler() = 0; |
There was a problem hiding this comment.
CellularContext is not always derived from AT_CellularContext. Any way to use here more generic interface than ATHandler?
There was a problem hiding this comment.
It's not now clear that handler reference may die when context is deleted. Should probably use reference counting similar to other interfaces?
There was a problem hiding this comment.
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.
|
@TeemuKultala Fyi, this needs a rebase |
|
Is this targeting 5.11 ? There's been rebase needed and reviewers ! |
|
@0xc0170 The target for this is 5.12 |
| @@ -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; | |||
There was a problem hiding this comment.
This needs to be in a SingletonPtr to avoid static memory wastage.
| _getReleaseMutex.lock(); | ||
| ATHandler *atHandler = _atHandlers; | ||
| while (atHandler) { | ||
| if (atHandler->get_file_handle() == fileHandle) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -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, | |||
There was a problem hiding this comment.
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).
| return NULL; | ||
| } | ||
|
|
||
| _getReleaseMutex.lock(); |
There was a problem hiding this comment.
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.
| return atHandler; | ||
| } | ||
|
|
||
| nsapi_error_t ATHandler::release(ATHandler *at_handler) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Timeout type mismatches the constructor.
| return atHandler; | ||
| ATHandler *AT_CellularDevice::get_at_handler() | ||
| { | ||
| return get_at_handler(NULL); |
There was a problem hiding this comment.
@kjbracey-arm Through this an ATHandler instance is available for use outside CellularDevice
There was a problem hiding this comment.
Ah, was just confused at what passing NULL was doing. See it now.
| return NULL; | ||
| } | ||
|
|
||
| _getReleaseMutex->lock(); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Ah, was just confused at what passing NULL was doing. See it now.
| @@ -83,6 +84,26 @@ class ATHandler { | |||
| */ | |||
| FileHandle *get_file_handle(); | |||
|
|
|||
| /** Get a new ATHandler instance, and update the linked list | |||
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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).
| while (atHandler) { | ||
| atHandler->set_at_timeout(_default_timeout, true); // set as default timeout | ||
| atHandler = atHandler->_nextATHandler; | ||
| if (_network) { |
There was a problem hiding this comment.
This could be iterator over ATHandlers.
|
@kjbracey-arm Can you rereview? |
|
CI started in the meantime |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
|
@TeemuKultala Question about this PR. Would it be possible for this to instead go into the |
ghost
changed the base branch from
|
@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. |
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