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

Stats collector; kotlin; mcumgr test mock framework #65

Merged
merged 14 commits into from
Feb 28, 2020
4 changes: 3 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
*/

buildscript {
ext.kotlin_version = '1.3.61'
repositories {
google()
jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:3.2.1'
classpath 'com.android.tools.build:gradle:3.5.3'
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
classpath 'com.dicedmelon.gradle:jacoco-android:0.1.4'
}
}
Expand Down
4 changes: 2 additions & 2 deletions gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#Mon Oct 01 11:43:39 CEST 2018
#Fri Feb 21 15:26:59 PST 2020
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.2-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-5.4.1-all.zip
2 changes: 1 addition & 1 deletion mcumgr-ble/gradle.properties
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
POM_ARTIFACT_ID=mcumgr-ble
POM_NAME=McuManager Ble
POM_PACKAGING=aar
POM_PACKAGING=aar
12 changes: 9 additions & 3 deletions mcumgr-core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

apply plugin: 'com.android.library'
apply plugin: 'kotlin-android'
apply from: rootProject.file('gradle/jacoco-android.gradle')

android {
Expand All @@ -26,19 +27,24 @@ android {

dependencies {

// Kotlin
implementation "androidx.core:core-ktx:1.2.0"
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"


// Annotations
implementation 'org.jetbrains:annotations:16.0.1'

// Logging
implementation 'org.slf4j:slf4j-api:1.7.25'

// Import CBOR parser
implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.9.6'
implementation 'com.fasterxml.jackson.core:jackson-core:2.9.6'
implementation 'com.fasterxml.jackson.core:jackson-databind:2.9.6'
implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.9.10'
implementation 'com.fasterxml.jackson.core:jackson-core:2.9.10'
implementation 'com.fasterxml.jackson.core:jackson-databind:2.9.10'

// Test
testImplementation 'junit:junit:4.12'
testImplementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.2"
}

apply from: rootProject.file('gradle/gradle-mvn-push.gradle')
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
package io.runtime.mcumgr.managers.meta

import io.runtime.mcumgr.McuMgrCallback
import io.runtime.mcumgr.exception.McuMgrErrorException
import io.runtime.mcumgr.exception.McuMgrException
import io.runtime.mcumgr.managers.StatsManager
import io.runtime.mcumgr.response.stat.McuMgrStatListResponse
import io.runtime.mcumgr.response.stat.McuMgrStatResponse
import java.util.concurrent.atomic.AtomicBoolean

/**
* Result of statistic collections.
*/
sealed class StatCollectionResult {
data class Success(val statistics: Map<String, Map<String, Long>>): StatCollectionResult()
data class Cancelled(val statistics: Map<String, Map<String, Long>>): StatCollectionResult()
data class Failure(val throwable: Throwable): StatCollectionResult()
}

/**
* Callback for statistic collections.
*/
typealias StatCollectionCallback = (StatCollectionResult) -> Unit

/**
* Non-blocking cancellable interface for cancelling an ongoing task.
*/
interface Cancellable {
fun cancel()
}
twyatt marked this conversation as resolved.
Show resolved Hide resolved

/**
* Collects stats from a device.
*/
class StatisticsCollector(private val statsManager: StatsManager) {

/**
* Collect stats from a single group by name.
*/
fun collect(groupName: String, callback: StatCollectionCallback): Cancellable {
return StatCollection(statsManager, callback).start(listOf(groupName))
}

/**
* Collect from a list of statistic group names.
*/
fun collectGroups(groupNames: List<String>, callback: StatCollectionCallback): Cancellable {
return StatCollection(statsManager, callback).start(groupNames)
}

/**
* List the stat group names from the device and collect each which intersects with the filter.
*/
fun collectAll(filter: Set<String>? = null, callback: StatCollectionCallback): Cancellable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You pulled in the Coroutines dependency but then utilizing legacy data flow paradigms. Is it because tools that rely on this library don't support modern data flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coroutines is currently used in the unit tests, I can make it a testImplemetation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why the external API is not coroutines is to allow for people to use the traditional callbacks if they want (like we do...). Building coroutines support on top of this API would be very simple needed.

Copy link
Contributor

@twyatt twyatt Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the plan is to make Coroutines a first class citizen, then perhaps it would be better to use Coroutines in the library then provide an "extension" library that wraps the Coroutine versions to provide the callback implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to have coroutines as a first-class citizen. The issue I see is that the current architecture for mcumgr does not include it whatsoever; so any coroutines addition to the current major version 0.x.x would simply be at a wrapping-api level. The crux of coroutine support would be at the transport interface.

I think we should plan a v1.x.x with coroutines built in all the way down to the transport interface and beyond, with a callback based api-wrapper (essentially the opposite of what we would do now).

In the end, I think adding corountes would be great, just not so in this PR.

val collection = StatCollection(statsManager, callback)
statsManager.list(object: McuMgrCallback<McuMgrStatListResponse> {

override fun onResponse(response: McuMgrStatListResponse) {
// Check for error response.
if (!response.isSuccess) {
callback(StatCollectionResult.Failure(McuMgrErrorException(response)))
return
}
// Filter statistic group names.
val groupNames = filter?.intersect(response.stat_list.toSet())?.toList()
?: response.stat_list.toList()
// Ensure group names in response.
if (groupNames.isEmpty()) {
callback(StatCollectionResult.Failure(
IllegalStateException("Statistic group list is empty.")
))
return
}
// Start collection
collection.start(groupNames)
}

override fun onError(error: McuMgrException) {
callback(StatCollectionResult.Failure(error))
}
})
return collection
}
}

/**
* Manages a single statistics collection.
*/
private class StatCollection(
private val statsManager: StatsManager,
private val callback: StatCollectionCallback
): Cancellable {

private val cancelled = AtomicBoolean(false)
private val started = AtomicBoolean(false)
private val result = mutableMapOf<String, Map<String, Long>>()

/**
* Start the stat collection for a given list of statistics groups.
*
* Start must only be called once per collection and must be provided at least one group to
* collect from. Otherwise this method will throw an error.
*
* @throws IllegalArgumentException If the stat collection has already been started.
*/
fun start(groupNames: List<String>): Cancellable {
check(started.compareAndSet(false, true)) { "Cannot call start() twice." }
if (groupNames.isEmpty()) {
callback(StatCollectionResult.Failure(IllegalArgumentException("List of group names is empty.")))
return this
}
if (cancelled.get()) {
callback(StatCollectionResult.Cancelled(result))
return this
}
collect(0, groupNames, callback)
return this
}

private fun collect(index: Int, groupNames: List<String>, callback: StatCollectionCallback) {
require(index in groupNames.indices) { "Index $index is out of range of groupList." }
statsManager.read(groupNames[index], object: McuMgrCallback<McuMgrStatResponse> {

override fun onResponse(response: McuMgrStatResponse) {
if (!response.isSuccess) {
callback(StatCollectionResult.Failure(McuMgrErrorException(response)))
return
}
result[response.name] = response.fields
when {
index == groupNames.size - 1 -> callback(StatCollectionResult.Success(result))
cancelled.get() -> callback(StatCollectionResult.Cancelled(result))
else -> collect(index + 1, groupNames, callback)
}
}

override fun onError(error: McuMgrException) {
callback(StatCollectionResult.Failure(error))
}
})
}

override fun cancel() {
cancelled.set(true)
}
}
14 changes: 14 additions & 0 deletions mcumgr-core/src/main/java/io/runtime/mcumgr/util/CBOR.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.cbor.CBORFactory;

import org.jetbrains.annotations.NotNull;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.util.HashMap;
Expand Down Expand Up @@ -56,4 +58,16 @@ public static Map<String, Object> toObjectMap(byte[] data) throws IOException {
ByteArrayInputStream inputStream = new ByteArrayInputStream(data);
return mapper.readValue(inputStream, typeRef);
}

public static <T> T getObject(@NotNull byte[] data, @NotNull String key, @NotNull Class<T> type) throws IOException {
ObjectMapper mapper = new ObjectMapper(sFactory);
return mapper.convertValue(mapper.readTree(data).get(key), type);
}

@NotNull
public static String getString(@NotNull byte[] data, @NotNull String key) throws IOException {
ObjectMapper mapper = new ObjectMapper(sFactory);
return mapper.readTree(data).get(key).asText();
}

}
44 changes: 0 additions & 44 deletions mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.java

This file was deleted.

35 changes: 35 additions & 0 deletions mcumgr-core/src/test/java/io/runtime/mcumgr/McuMgrImageTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package io.runtime.mcumgr

import io.runtime.mcumgr.image.McuMgrImage
import org.junit.Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import org.junit.Test
import kotlin.test.Test

import java.io.ByteArrayOutputStream
import java.io.InputStream

class McuMgrImageTest {

@Test
fun `parse image without protected tlvs success`() {
val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!
?: throw IllegalStateException("input stream is null")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
?: throw IllegalStateException("input stream is null")

val imageData = toByteArray(inputStream)
Comment on lines +12 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: could make these more readable with a quick refactor.

fun String.readByteArray() =
    McuMgrImageTest::class.java.classLoader!!
        .getResourceAsStream(this)
        .toByteArray()
Suggested change
val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-no-prot-tlv.img")!!
?: throw IllegalStateException("input stream is null")
val imageData = toByteArray(inputStream)
val imageData = "slinky-no-prot-tlv.img".readByteArray()

McuMgrImage.fromBytes(imageData)
}

@Test
fun `parse image with protected tlvs success`() {
val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")
?: throw IllegalStateException("input stream is null")
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick

Suggested change
val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")
?: throw IllegalStateException("input stream is null")
val inputStream = this::class.java.classLoader?.getResourceAsStream("slinky-prot-tlv.img")!!

val imageData = toByteArray(inputStream)
McuMgrImage.fromBytes(imageData)
}

private fun toByteArray(inputStream: InputStream): ByteArray {
val os = ByteArrayOutputStream()
val buffer = ByteArray(1024)
var len: Int
while (inputStream.read(buffer).also { len = it } != -1) {
Comment on lines +26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick

Suggested change
private fun toByteArray(inputStream: InputStream): ByteArray {
val os = ByteArrayOutputStream()
val buffer = ByteArray(1024)
var len: Int
while (inputStream.read(buffer).also { len = it } != -1) {
private fun InputStream.toByteArray(): ByteArray {
val os = ByteArrayOutputStream()
val buffer = ByteArray(1024)
var len: Int
while (read(buffer).also { len = it } != -1) {

os.write(buffer, 0, len)
}
return os.toByteArray()
}
}
Comment on lines +1 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability, the combination of the above suggestions:

class McuMgrImageTest {

    @Test
    fun `parse image without protected tlvs success`() {
        McuMgrImage.fromBytes("slinky-no-prot-tlv.img".readByteArray())
    }

    @Test
    fun `parse image with protected tlvs success`() {
        McuMgrImage.fromBytes("slinky-prot-tlv.img".readByteArray())
    }
}

private fun String.readByteArray() =
    McuMgrImageTest::class.java.classLoader!!
        .getResourceAsStream(this)
        .toByteArray()

private fun InputStream.toByteArray(): ByteArray {
    val os = ByteArrayOutputStream()
    val buffer = ByteArray(1024)
    var len: Int
    while (read(buffer).also { len = it } != -1) {
        os.write(buffer, 0, len)
    }
    return os.toByteArray()
}

Loading