Conversation
|
@philips77 Let me know what you think of this design. No need for a full code review; just wanted to get your input on the restructuring. |
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
============================================
- Coverage 14.80% 13.96% -0.84%
+ Complexity 113 99 -14
============================================
Files 64 59 -5
Lines 2128 1905 -223
Branches 217 184 -33
============================================
- Hits 315 266 -49
+ Misses 1776 1610 -166
+ Partials 37 29 -8 |
|
I like the new structure. Also, I went quickly through the code and looks very good. Eager to see the end result. Do you plan to make the API backwards compatible, or as it's 1.0 you don't have to carry this baggage? And, are you planning on adjusting the sample app when libraries are complete? |
|
I think backwards compatibility might be off the table due to the major type changes (e.g. McuMgrResponse, McuManager, etc.) and the package name changes. However, mimicking the previous callback style APIs to lessen the effect of updating from 0.x to 1.x should be easy.
Yes, I am. Although I think that will be the last item on the TODO list. |
twyatt
left a comment
twyatt
left a comment
There was a problem hiding this comment.
Overall awesome job! Love where this is going. Some initial thoughts from a quick review pass:
Does serialization actually need to be a separate module? Will other modules need to use that code (or is it specific to core)?
I'd recommend utilizing binary-compatibility-validator to monitor your API surface.
| data class Response(val data: ByteArray, val offset: Int, val length: Int?) | ||
| } | ||
|
|
||
| @Throws |
There was a problem hiding this comment.
I imagine this function doesn't throw since exceptions would be propagated through the flow, not at the call site.
| fun Downloader.flow( | ||
| capacity: Int | ||
| ) = channelFlow { |
There was a problem hiding this comment.
I think I kind of understand what's going on here, but if so, then it doesn't seem you need a Flow.
Flow is useful when you want to provide a lazy on-demand stream of data, but from what I can tell, you'll explicitly want to perform the upload. i.e. Standard Coroutines will accomplish what you want and be a lower cognitive lift to understand the code.
There was a problem hiding this comment.
Sounds good. I think will implement this with a returned receive channel instead.
There was a problem hiding this comment.
Not sure returning a Channel is the right approach either. Either that or I'm very much misunderstanding the purpose of this function.
Don't you just want to call a function and perform some downloads?
Why does that need to be "subscription" based? Why can't it be a suspend function (or CoroutineScope extension function)?
Unless this is an ongoing data stream? Whereas downloads could "appear" at any given time, and the Flow allows a library consumer to subscribe and be informed of these sporadic download events?
There was a problem hiding this comment.
I'm not set on using a Flow by any means, however I'd like the transfer to be a stream for progress callbacks which can drive a UI element (e.g. image upload used in DFU).
There was a problem hiding this comment.
Oh gotchya. For the purposes of propagating status/state a data stream does make sense.
What about this for an API (as an example; actual types may differ)?:
class Downloader(private val resource: URI) {
// private MutableStateFlow under the hood, that emits values from 0-100
val progress: Flow<Int>
// suspends until download is complete
suspend fun download(): ByteArray
}Here, a call to download will suspend until complete. If developer wants status of the download (i.e. for UI updating purposes) then they can optionally subscribe to progress.
You can make progress more fancy, with a sealed class that represents things like InProgress(percent: Float), Complete or Failure. The sealed class from the progress field probably wouldn't be necessary if you throw (or return domain-specific result) from the download function. Best to keep things simple and from the progress Flow only propagate events that are leading up to the termination of the download. The download function should be your source of termination event.
Subscribing or unsubscribing to progress has no effect on the download (so is fine to subscribe from, for example, a LiveData that may unsubscribe and re-subscribe based on lifecycle; i.e. device rotation).
Your download function can throw an exception on failure, or return a domain-specific Result object; up to you.
There was a problem hiding this comment.
You can get ultra fancy with something like:
suspend fun download(
resource: URI,
progressHandler: ((Int) -> Unit)? = null
): ByteArray {
val downloader = Downloader(resource)
return coroutineScope {
val job = downloader.progress.onEach { progressHandler?.invoke(it) }.launchIn(this)
val result = downloader.download()
progressHandler?.invoke(100)
job.cancel()
result
}
}It's usage could like:
val bytes = download(uri) { progress -> /* todo: update UI */ }
@twyatt Whatever implements To sum up, most users don't need access to anything in |
Got it. Makes sense if developers extending functionality of the library need access to the serialization to have it as a separate module. Thanks for clarifying. 👍 |
|
@twyatt I have updated the upload and download classes with a new API, added tests, and fixed a few bugs. Notes:
Image upload is used as an example of API suspend fun uploadImage(
data: ByteArray,
windowCapacity: Int = 1,
progressHandler: ((UploadProgress) -> Unit)? = null
): McuMgrResult<Unit>Example Usage uploadImage(imageData, 5) {
// Update progress UI
}.onErrorOrFailure {
// Handle failure
}On a side note, I noticed in your more recent projects that the way gradle dependencies are handled has changed. E.g. dependencies are now listed in |
Not sure if it's the "new hot way" but it's just something I adopted to organize things a bit more. The If you also find it to be a more organized/cleaner approach to dependency management in Gradle, then yay! Totally up to you though. |
| val transmitOffset = offset | ||
| val chunkSize = getChunkSize(data, offset) | ||
|
|
||
| window.acquire() |
There was a problem hiding this comment.
Which transport is this used for?
For BLE there won't be any advantage to parallelizing the writing of chunks. Is there a transport that is more efficient by sending multiple chunks at the same time?
Am I understanding correctly what window is for?
There was a problem hiding this comment.
Not for serialization, buf for dynamic flow control. We only want to have up to capacity number of active transactions (requests/responses) at a time in order to not overload the device into OOM. Further, if we receive an error for a request, we want to reduce the window size.
WindowSemaphore is the class which encapsulates this logic. In short, the window is initialized with a capacity which is the max size the window can reach. The size starts at 1 and grows by 1 with every successful request until either (a) the capacity is reached, or (b) a request errors out. On error (b) the window shrinks by one and a flag is set such that the window will no longer grow.
Example window capacity=4
-->== send request<--== receive response
--> id=1
<-- id=1, result=success (size = 2)
--> id=2
--> id=3
<-- id=2, result=success (size = 3)
--> id=4
--> id=5
<-- id=3 result=success (size = 4)
--> id=6
--> id=7
<-- id=4 result=success (size = 4) // Reached capacity
--> id=8
<-- id=5 result=success (size = 4)
--> id=9
<-- id=6 result = FAIL (size = 3) // Shrink and retry
--> id=6
<-- id=7 result = success (size = 3) // id=6 error means no more growth
<-- id=8 result = success (size = 3)
--> id=10
<-- id=9 result = success (size = 3)
--> id=11
<-- id=6 result = success (size = 3) // recover from error
...
While I was writing up this timeline I noticed a bug in the current implementation which I justl fixed.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I see. So the remote device can concurrently process these chunks while more chunks are coming in? And processing takes some time, hence why we're able to send multiple chunks before the remote has replied?
Exactly.
How much faster is it to send multiple chunks concurrently?
Even using a capacity of 2 (two concurrent chunks) speeds up the upload by a factor of 2. As the capacity is increased the speedup flattens out, but e.g. capacity = 5 makes the image upload (even with our security protocols) speed up to less than 50 seconds (from 120 seconds).
| @Throws | ||
| suspend fun upload( | ||
| uploader: Uploader, | ||
| progressHandler: ((UploadProgress) -> Unit)? = null |
There was a problem hiding this comment.
This feels like Java callback style when you have Coroutines to provide data streams with all its fancy operators.
In fact, it looks like you're wrapping a nice Coroutines API in this callback style function.
This function feels like a good candidate for a separate module where you bridge the Coroutine and Java world. Doesn't feel like it belongs in this module.
Instead, you could have:
class McuManager {
// ...
fun fileUploader(
data: ByteArray,
fileName: String,
windowCapacity: Int = 1
) = FileUploader(data, fileName, this, windowCapacity)
// ...
}Users of library can easily call and use it as they see fit:
val mcumgr: McuManager
// When progress isn't needed.
mcumgr.fileUploader(...).upload() // Suspends until complete.If progress is desired then one possible usage could look like:
val mcumgr: McuManager
val uploader = mcumgr.fileUploader(...)
uploader
.progress
.transformWhile { progress ->
emit(progress)
progress != 100
}
.onEach { ... }
.launchIn(scope)
uploader.upload() // Suspends until complete.transformWhile could be baked into the provided progress Flow, simplifying the above example.
| data class Error<T>(val code: Response.Code) : McuMgrResult<T>() | ||
|
|
||
| data class Failure<T>(val throwable: Throwable) : McuMgrResult<T>() |
There was a problem hiding this comment.
It's not immediately clear what the difference is between an error vs. a failure?
Is there a need for the distinction? How would a consumer handle these differently?
There was a problem hiding this comment.
The difference is an error response vs a total failure. E.g. a response with one field {"rc":5} would be an error response, while a ble disconnect would be a failure.
The reason to make the distinction is because, like HTTP requests, sometimes we expect a response to return an error. E.g. when downloading a core, the device may respond with Response.Code.NoEntry effectively telling us that there is no core available to download. This is not a failure, nor does it conform to the expected download response format, hence being a separate entity.
For most callers, the distinction is moot. However, it would be incorrect to lump errors and failures together, since doing so may cause failures which are not truly a failure.
There was a problem hiding this comment.
Gotchya. It'd be good to name them (if possible) to be more specific.
For example, Retrofit will throw either HttpException which indicates that an HTTP response was received (it was just not in the 2xx range) or an IOException to indicate a network failure. The naming makes it clear to the library consumer what they mean and how to react to them.
Failure vs. Error doesn't provide the same meaningful naming. If a more specific naming can't be found, then KDoc should be added to them to make it clear to the library consumer what their purpose is.
| import com.juul.mcumgr.McuMgrResult.Success | ||
| import com.juul.mcumgr.message.Response | ||
|
|
||
| sealed class McuMgrResult<out T> { |
There was a problem hiding this comment.
This result class doesn't feel domain-specific. It looks like just a general result class.
That's not necessarily a problem, but it may be unnecessarily adding a lot of complexity as opposed to just propagating exceptions.
If we're just wrapping exceptions for the purpose of wrapping in a general result object, consumers of the library could do that with runCatching. By wrapping in a result you've added overhead and consumers have to unwrap if they want the exception. Seems more optimal to give the choice of creating the wrapped objects?
Now if this were more domain-specific (i.e. some of the sealed sub classes represented result states, such as upload failed due to timeout, or specific responses from the remote device, then suddenly this result is very informative/valuable).
There was a problem hiding this comment.
Good point. I feel somewhat uncomfortable making public APIs so throw heavy, but it does provide a broader API for consumers to use.
Domain specific results might be better for wrapping classes (e.g. Downloader, Uploader) which perform additional logic on top of the base mcumgr command.
I'll keep McuMgrResult around to use for now, but I wont use it in the public API. If at the end, McuMgrResult is no longer useful, I can remove it.
There was a problem hiding this comment.
I feel somewhat uncomfortable making public APIs so throw heavy
Agreed, and I definitely don't want to discourage use of result objects as they are more idiomatic (vs. throwing exceptions).
If possible, it would be preferred to return a domain-specific result object.
In this case, the result object just wasn't domain-specific. It looked like part of the design challenge is that this result object was wrapping your transport functions; which themselves don't provide a well defined possible error states (so we lean on the very general throw mechanics). Perhaps clearly defined failure states at the transport layer could help form a domain-specific result object for your transport functions. Then at the mcumgr layer (that calls the transport functions) you could easily map those results, or basically handle the ones than can be handled and then return the mcumgr-specific result object?
You'll probably also find, that if you have a domain-specific result object, then you don't need to lean on generics anymore.
| val transmitOffset = offset | ||
| val chunkSize = getChunkSize(data, offset) | ||
|
|
||
| window.acquire() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@philips77, @bgiori is no longer @JuulLabs. Internally we are no longer using this library, so we do not have any active developers working or maintaining it. @philips77 I believe you already have write access to this repo; feel free the act as the primary maintainer (if you have the time) — I can look into getting you additional access if needed as well. |
|
Hi @twyatt, Thank you for the information. I think it will be easier for us to maintain only our current fork and release from there, perhaps deprecating Android and iOS repos on your organization. |
I vaguely remember there was a bit more to do on this PR, but I really don't know for certain.
Sounds good to me. What is the URL for your Android and iOS repos so we can list it in this project's repo (to help redirect users to a maintained version)? |
3 participants
Brief Notes (Updated)
core: Contains API and transport interface definition. Also contains the new, faster window upload and download.serialization: Contains encoder, decoder, CBOR object mapper and base message type.Library Structure
Core
Serialization
Transport
The new transport interface has not been officially implemented yet. The closest thing is the mock transport in the tests, here:
https://github.com/JuulLabs-OSS/mcumgr-android/blob/1.x.x/core/src/test/kotlin/mock/Transport.kt
Message Format
Mcumgr messages are made up by an 8-byte header and CBOR payload. The actual serialized encoding, however, depends on the message format being used by the transport.
Header
The header is always 8-bytes long and contains 6 distinct fields; of which 5 are actually used.