MicroBitUtilityService - replaces PR 178 #287
Conversation
| * a temporary fiber is created as necessary. | ||
| */ | ||
| #ifndef MicroBitUtilityService_FIBER | ||
| #define MicroBitUtilityService_FIBER 0 |
There was a problem hiding this comment.
Is there a reason to hold both these implementations in the final revision @martinwork? or shall we just pick one? (e.g. the event based approach?)
There was a problem hiding this comment.
Yes please pick one - whichever you think is the best idea.
|
@finneyj I updated from master and removed the MicroBitUtilityService_FIBER option. It compiles, but I haven't tested it yet. |
|
@finneyj I tested it now, using https://github.com/martinwork/microbit-v2-samples/tree/test-mydata. I had to modify MicroBit.cpp for the panic 071 problem. Would you like me to add a commit for that here? Make a separate PR? |
|
Thanks @martinwork! Yes please - working up a PR for the 071 panic would be fabulous, thank you! |
| { | ||
| if ( workspace) | ||
| { | ||
| if ( workspace->lock) |
There was a problem hiding this comment.
is this safe @martinwork? If I understand correctly, this code will silently ignore incoming requests if a request is already being processed? Is that conventional for BLE apps? Seems quite lossy?
There was a problem hiding this comment.
I believe my thinking was that the each request would expect a reply (both current ones do), and the client wouldn't send multiple requests.
There was a problem hiding this comment.
I think that's ok - we just need to record somewhere in docs that we don't permit overlapping transactions.
| if ( workspace->replyState == replyStateClear) | ||
| { | ||
| int block = min( request->batchlen, sizeof( reply_t) - offsetof( reply_t, data)); | ||
| int result = log.readData(workspace->reply.data, request->index, block, (DataFormat) request->format, request->length); |
There was a problem hiding this comment.
Does this mean we're very likely to see a sequence of 20 byte read operations from the log @martinwork? That might add quite a bit of latency, compared to one larger read operation?
There was a problem hiding this comment.
Won't most of the 20 byte reads be from uBit,log's cache? I suppose there's a fair bit of code for each read to arrive at the memcpy.
There was a problem hiding this comment.
yes, you're quite right! I forgot we applied the block cache from the file system API into the log reads... All good there then.
I'm not worried about the cache lookup/memcpy cost. It will be insignificant compared to all the BLE/I2C latency...
|
Generally looks good to me @martinwork! Thanks for the contribution! I do think we should keep this aligned with the USB based mailbox implementation we drafted for reading the log in due course - just so that we can keep and consistent API for both USB and BLE based transactions. This is already pretty close though, and seeing as we don't want to hold this up any longer, I suggest we tackle that down the line - perhaps by aligning the USB implementation with your BLE one here... |
|
Thanks for checking it @finneyj |
|
I'm happy to merge then @martinwork - thanks for the work and quick responses. Unless you have any further thoughts @JohnVidler? |
A fresh branch from current master.
Replaces #178
TODO: How should the service and characteristic UUIDs determined?
This currently uses the micro:bit base UUID plus 1 and 2.
TODO: MICROBIT_ID_UTILITY - ID definitions have moved into codal-core
MicroBitUtilityService.h/cpp - BLE service providing replies to simple client requests
MicroBitUtilityTypes.h - Details of the service requests that support reading MicroBitLog data
MicroBitCompat.h - Define MICROBIT_ID_UTILITY for service events
MicroBitConfig.h - Macros to control creation in pairing / application mode, default pairing mode only.
MicroBit.cpp - Create service according to config macros. To create in BLEManager::init would need to pass MicroBitLog.
MicroBitBLEChar.cpp - Changed DMESGF to DMESG.