Composition of specifications and devices #13
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "dev-maxim"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implemented builders for PropertyDescriptor and ActionDescriptor. Some typos have been fixed. Introduced a new hierarchical device structure based on CompositeDeviceSpec and ConfigurableCompositeDevice. They draw ideas from DeviceSpec and DeviceBySpec. This is a prototype implementation, and further refactoring is expected. The key feature is clear composition of device specifications for a declarative description of devices and their behavior. Moreover, the API for parent and child devices (specs) is the same.
Please fix MR name to better reflect what it does.
Also please add examples of new API usage both in code and in MR description.
WIP: dev-maximto WIP: Composition of specifications and devicesAdded examples of new API usage - CompositeControlTest based on Analyzer device with subdevices. In specification, define "by childSpec" for passing child device specification. In device, define "by childDevice" for getting child device. For now not properly working, not all tests are passed.
@ -0,0 +42,4 @@
/**
* Defines how child device lifecycle should be managed.
*/
public enum class LifecycleMode {
Seems like a bit of overengeneering to me. Could you provide examples, where all of theese are needed? Core API should be minimal
@ -0,0 +70,4 @@
public fun <D: ConfigurableCompositeControlComponent<*>> getSpec(name: Name): CompositeControlComponentSpec<D>?
}
public class ComponentRegistryManager : AbstractPlugin(), ComponentRegistry {
This seems to be a nice idea, but rather for remote configuration data base. What is the use case for this class locally? Should it be in the core?
@ -0,0 +18,4 @@
/**
* Create a [MetaConverter] for enum values
*/
public fun <E : Enum<E>> createEnumConverter(enumValues: Array<E>): MetaConverter<E> = object : MetaConverter<E> {
Scope polution here. Please move to MetaConverter.Companion
@ -0,0 +18,4 @@
// ---------------------- Device Specifications ----------------------------------
public object StepperMotorSpec : CompositeControlComponentSpec<StepperMotorDevice>() {
public val position by intMutable(
The naming convention should be discussed
@ -0,0 +110,4 @@
// ---------------------- Device Implementations ----------------------------------
// Implementation of Stepper Motor Device
public class StepperMotorDevice(
How is it different from regular Spec?
The main problem for me here is that it is not clear, how Composite devices are better than regular ones. I would like to mimimize the API border for the core.
@kolpakov.mm could you split this MR on two:
There are lot of questions. The main issue I see right now is that significant part of distributed functionality that was intended for Magix is now re-implemented from scratch. Since the module is not not in the core, I recommend import magix and magix-utils and implement all this functionality on top of Magix.
The core feature is you can add new types of Magix message: just add a type, payload content and new
MagixMessageFormat
. It could elegantly solve all the problems with system events, log events etcetera.@ -0,0 +1,3287 @@
@file:Suppress("MemberVisibilityCanBePrivate", "unused")
This file is too large. Please split it into separate classes.
@ -0,0 +24,4 @@
import kotlin.time.Duration.Companion.seconds
/** Global configuration for controls system. */
public class ControlsConfiguration(
Configurations like this should be passed as DeviceManager plugin parameters. Better to convert them into DeviceManager extensions. This is needed to make the whole system installable from configuration.
Global ControlsConfiguration has been removed. Now the plugin is DeviceManagerConfig, which is registered in the context and is accessible via context.deviceManagerConfig.
@ -0,0 +56,4 @@
* @param T The type parameter of the deferred result.
* @return The completed value if available and not cancelled, or `null` otherwise.
*/
private suspend fun <T> Deferred<T>.awaitCompletedOrNull(): T? =
This is strange extension. It will return null if task is completed.
Deleted
@ -0,0 +75,4 @@
/**
* Base class for all device-related exceptions
*/
I think exceptions should be moved to a separate file
@ -0,0 +76,4 @@
/**
* Base class for all device-related exceptions
*/
public sealed class DeviceException(
Shouldn't it be done via inheritance?
Regarding the DeviceException organization, I'm not sure yet how to do it better. See the last commit
@ -0,0 +153,4 @@
/**
* EventBus interface for publishing and subscribing to application-level events.
*/
public interface EventBus {
Does it make sense to dublicate the whole bus infrastructure when we already have Magix bus? They are implemented on a different level: Magix operates on inter-device level, but that makes it more flexible. Also, it allows remote monitoring. At this point it does not make a lot of sense keeping two separate busses.
EventBus and TransportAdapter removed. Made MessageBus with InMemoryMessageBus and MagixMessageBus implementations. DeviceHubManager uses MessageBus (by default MagixMessageBus if Magix is available, otherwise InMemoryMessageBus). To make it easier to use without Magix in base
@ -0,0 +176,4 @@
/**
* Event emitted when a message is sent through the transport
*/
public data class MessageSentEvent(val message: DeviceMessage) : Event
Could it be replaced with
EmptyDeviceMessage
?Actually I've removed it now
@ -0,0 +230,4 @@
* TransportAdapter interface for distributed communications.
* This abstraction allows plugging in different transport mechanisms.
*/
public interface TransportAdapter {
This is a direct replica of Magix bus. I think, Magix endpoint should be used instead.
I removed it too
@ -0,0 +303,4 @@
/**
* Represents an undoable action for transaction rollback
*/
public interface UndoableAction {
Better to name it reversible. Or it reads as action that cannot be done.
Also reversing actions is a complex concept, it should be discussed separately.
Interface renamed to ReversibleAction
@ -0,0 +319,4 @@
* Represents a transaction context with a unique ID.
* This is used to track and manage transactions.
*/
public class TransactionContext(
Tranactions are part of storage. I think this should be a complete separate thing, maybe on storage side.
@ -0,0 +350,4 @@
/**
* Interface for managing transactional operations.
*/
public interface TransactionManager {
At the very least, tranactions should be mover into a separate file. And in general it should be done in a separate MR.
@ -0,0 +488,4 @@
/**
* Interface for publishing metrics.
*/
public interface MetricPublisher {
It is better to do that in a separate type of Magix message
MetricPublisherImpl now publishes MetricMessages (with subtypes Value, Counter, Duration, etc.) via MessagingSystem, which can be Magix
@ -0,0 +596,4 @@
/**
* Thread-safe atomic reference implementation.
*/
public class AtomicReference<T>(initialValue: T) {
Please use https://github.com/Kotlin/kotlinx-atomicfu for that, do not reinvent the wheel.
Fixed, now atomic is from the library.
@ -0,0 +658,4 @@
/**
* Interface for loading external device configurations.
*/
public interface ExternalConfigurationProvider {
I think it could be done with
MagixRegistry
frommagix-utils
module.Interface removed. Configuration is now passed via Meta
@ -0,0 +1032,4 @@
*
* @param CD The type of the child device.
*/
public interface ChildComponentConfig<CD : ConfigurableCompositeControlComponent<CD>> {
We need to try to follow DataForge ideology here. Namely that everything could be created using context and metadata. Using any stateful objects for configuration could break automatic object creation.
Added fromMeta method to create config from Meta, if that's appropriate.
@ -0,0 +1048,4 @@
*
* @param D The device type this spec applies to.
*/
public interface CompositeDeviceSpec<D : ConfigurableCompositeControlComponent<D>> {
Could it inherit regular DeviceSpec?
There's a class there, and here's an interface, so I didn't touch it for now
@ -0,0 +1543,4 @@
/**
* Manager for health checks across devices.
*/
public class HealthCheckManager(
This should be movedd to a separate file. I also propose to use Magix for that. Since now you are not in the core, you can import Magix here.
@ -0,0 +1647,4 @@
/**
* Manages error handling for devices.
*/
public class ErrorHandlingManager(
This also should be separated and probably moved on top of magix. It is hard to implement distributed device registry, but I wonder if it is needed.
@ -0,0 +1978,4 @@
/**
* A [MutableSharedFlow] for system-level messages.
*/
public abstract val systemBus: MutableSharedFlow<SystemLogMessage>
Why do you need separate busses for different messages? Just create single bus with different types of magix messages.
Replaced with MessagingSystem / MessageBus. Different message types can be accessed via the required flows (getDeviceLogMessages, getSystemLogMessages)
@ -0,0 +10,4 @@
/**
* Create a [MetaConverter] for enum values using [reified] type with an option to ignore case.
*/
public inline fun <reified E : Enum<E>> createEnumConverter(ignoreCase: Boolean = false): MetaConverter<E> {
There is already MetaConverter.enum for that. Please explain why it is not applicable.
Here I am being a bit clever, perhaps, it's just that in the original in readOrNull it's somehow strange, an error may pop up, and not "null" in return. For me it returns null, and it works taking into account ignoreCase. But I'm not sure that this is very important or rather, yes. I just investigated.
Should be moved to MetaConverter.Companion to avoid global scope pollution.
@ -219,1 +219,4 @@
@Serializable
@SerialName("system.log")
public data class SystemLogMessage(
How is it different from DeviceLogMessage?
According to the old idea, SystemLogMessage is for system-level logs, and DeviceLogMessage is for logs of a specific device (sourceDevice) and can carry Meta. Not sure what is the best idea, maybe combine it with the "level" field (device/system).
@ -11,0 +11,4 @@
/**
* The device is newly created and has not started yet.
*/
INITIAL,
Maybe INIT?
@ -6,3 +6,3 @@
import space.kscience.dataforge.meta.MetaConverter
internal object DeviceMetaPropertySpec : DevicePropertySpec<Device, Meta> {
public object DeviceMetaPropertySpec : DevicePropertySpec<Device, Meta> {
Is it necessary?
No)
Then put it back.
@ -94,3 +94,4 @@ include(
":demo:constructor",
":demo:device-collective"
)
include("controls-composite-spec")
Add this to the list above. Before demos
Fixed
WIP: Composition of specifications and devicesto Composition of specifications and devicesПривет. Я посмотрел изменения, и у меня есть несколько вопросов:
The changes in the Devica Message hierarchy are not approved. Otherwise looks fine.
@ -0,0 +22,4 @@
implementation(projects.magix.magixRsocket)
implementation(projects.magix.magixZmq)
implementation(projects.controlsMagix)
implementation("org.jetbrains.kotlinx:atomicfu:0.27.0")
Is it necessary?
@ -19,3 +20,3 @@
@Serializable
public sealed class DeviceMessage {
public sealed class Message {
Why do you need this? To avoid writing comment? Just leave it blank. I do not think this change is necessary.
Checkout
From your project repository, check out a new branch and test the changes.