Refactor URC handling #16
Conversation
Now uses a vector to store individual URC handlers, and iterate through each one by one Also uses a separate vector for the URC strings within pruneBacklog This should make the URCs easier to maintain and expand, particularly within derived classes
Pending #16, this should work after that gets merged
| @@ -313,445 +328,525 @@ bool UBX_CELL::bufferedPoll(void) | |||
| return handled; | |||
| } // /bufferedPoll | |||
|
|
|||
| // Parse incoming URC's - the associated parse functions pass the data to the user via the callbacks (if defined) | |||
| bool UBX_CELL::processURCEvent(const char *event) | |||
| bool UBX_CELL::urcHandlerReadSocket(const char* event) | |||
There was a problem hiding this comment.
This is kind of where I was heading with the u-blox GNSS library refactoring. Except I was trying to go one step further and not define methods like this individually. Instead is there a more elegant way where you could have a single common add-handler method and pass in: a pointer to the const char AT command; the search string; and some kind of common way of passing the data parsed / extracted from the URC to the callback. I was heading in the direction of using const uint8_t * for all callbacks, and provide helper methods to extract the individual 'fields' inside the callback...
In the case of this library, there are relatively few of these. And relatively few callbacks (each with their own parameter). So, I'm OK with you merging this as-is. But, for bonus points, you could spend five minutes thinking about that possible more elegant solution?!
(In the case of the GNSS library, I absolutely need the elegant method because there are so many individual messages (== URCs), each containing many many fields... That's where I was last week when I threw the towel in (again)...)
There was a problem hiding this comment.
By eye, this looks great. I need to test it on SARA-R5. But I think I need you to merge it before I test it...
Please spend (no more than) 5 mins thinking about the above, then go ahead and merge it.
There was a problem hiding this comment.
Thanks Paul! I agree, there's definitely better solutions to this (eg. there's a lot of repeated code, such as searching for the URC strings or the values that follow). I do have a few ideas on how to further improve this (and other aspects of this library), but the goal right now is to just get it working "good enough" with our current products. If desired, we can always refactor it later.
URCs need to get handled differently, for a couple reasons:
RINGURC, but the current implementation doesn't really allow for that, at least not easily.processURCEvent()is very long, and adding new handlers requires remembering to updatepruneBacklog()too (I almost missed it myself).This new implementation splits up
processURCEvent()into individual handler methods for each URC. In the constructor, those handlers are added to a vector using the newaddURCHandler(). ThenprocessURCEvent()simply iterates through each handler, and only returns true if the event gets handled. This enables subclasses to add URC handlers within their constructors, and everything should be happy. This was @gigapod's suggestion, seems to work fine.Additionally,
pruneBacklog()needs to be aware of what URC strings are "actionable events", so I added a second vector to remember those. They're set when callingaddURCHandler().Now to handle a new URC, you must create a method to do so, then add it (and the string it's looking for) with
addURCHandler(). This should be more maintainable moving forward, and allow subclasses to easily add new URC handlers.