hmon: Define configuration schema - #340
Conversation
GitHub Actions
Waiting
GitHub Actions
Waiting
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
8c18416 to
7863fe8
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
7863fe8 to
032724b
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
eduard-moskalchuk
changed the title
| version_minor: uint32 = 0 (id:1); | ||
| supervisor_api_cycle_ms: uint32 (id:2); | ||
| internal_processing_cycle_ms: uint32 (id:3); | ||
| thread_parameters: ThreadParameters (id:4); |
There was a problem hiding this comment.
Not related to these changed, but I would be curious @eduard-moskalchuk if this thread configuration is working in your context.
I would assume in production environment application process will not have the privileges to changes the affinity or scheduling policy / priority for any of its threads.
There was a problem hiding this comment.
Probably not. In QNX some secpol abilities would be required for that, and most applications don't have them.
032724b to
4cb138a
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
SimonKozik
left a comment
SimonKozik
left a comment
There was a problem hiding this comment.
It is difficult for me to comment on the LogicMonitor, StateNode, HeartbeatMonitor, DeadlineMonitor and DeadlineEntry definitions without seeing an example configuration.
I would appreciate if we could have an example configuration (for all possible configuration parameters), so we could better asses the schema.
| "supervisor_api_cycle_ms": { | ||
| "type": "integer", | ||
| "minimum": 1, | ||
| "description": "Cycle duration in ms for supervisor API notifications" | ||
| }, |
There was a problem hiding this comment.
LM is using seconds as the measurement unit of time (e.g., '0.5' for 500 milliseconds). I would be tempted to suggest that we should use the same approach for health monitoring.
This way integrator will be not restricted with precision they would like to use. Encoding precision in parameter name will force configuration change, when we would allow different precision.
Additionally it may be useful to define time as a reusable type. This type can be then used by supervisor_api_cycle, internal_processing_cycle and TimeRange
There was a problem hiding this comment.
If we use floating point numbers, we may have to prove that we do it in a safe way. An activity that can be avoided. Please see the issue #345
4cb138a to
69fdfab
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
69fdfab to
06d60cb
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
@SimonKozik I added an example to the PR description. Would it make sense to push it as a file as well? |
@eduard-moskalchuk Thanks for the example. I would definitely recommend to store it alongside schema (this is what LM also does). Does it make sense to get rid of the tags? For me it is just extra typing for the user, without added value to the information that is stored. An example could looks like this: {
"deadline_monitors": {
"control_loop_deadlines": {
"read_inputs": {
"min_ms": 1,
"max_ms": 5
},
"publish_outputs": {
"min_ms": 2,
"max_ms": 10
}
},
"communication_deadlines": {
"receive": {
"min_ms": 1,
"max_ms": 3
},
"transmit": {
"min_ms": 2,
"max_ms": 8
}
}
}
} |
| } | ||
| }, | ||
| "additionalProperties": false, | ||
| "definitions": { |
There was a problem hiding this comment.
| "definitions": { | |
| "$defs": { |
In 2020-12 schema, the recommended location for reusable schema fragments is $defs
|
|
||
| /// Scheduler policy for the monitoring thread. | ||
| enum SchedulerPolicy : byte { | ||
| Other = 0, |
There was a problem hiding this comment.
Does this need to be adapted as well to the new names POSIX_OTHER?
I assume you want to directly compile the json to flatbuffer without any conversion script
| }, | ||
| "description": "CPU core IDs the thread can run on" | ||
| }, | ||
| "stack_size": { |
There was a problem hiding this comment.
Should we put the unit into the name, as we did with the timing values (_ms)?
e.g. stack_size_bytes?
| table HealthMonitorConfig { | ||
| schema_version: uint32 = null; | ||
| supervisor_api_cycle_ms: uint32; | ||
| internal_processing_cycle_ms: uint32; |
There was a problem hiding this comment.
Are supervisor_api_cycle_ms and internal_processing_cycle_ms optional values?
Currently, flatbuffer will return 0 here if not configured.
This might be OK since 0 will be invalid.
Though using = null may be more explicit if its really optional
|
|
||
| /// Root configuration for a HealthMonitor instance. | ||
| table HealthMonitorConfig { | ||
| schema_version: uint32 = null; |
There was a problem hiding this comment.
Is the schema version really optional?
4 participants
Example JSON configuration:
{ "schema_version": 1, "supervisor_api_cycle_ms": 100, "internal_processing_cycle_ms": 50, "thread_parameters": { "policy": "POSIX_FIFO", "priority": 20, "affinity": [0, 1], "stack_size": 262144 }, "deadline_monitors": [ { "monitor_tag": "control_loop_deadlines", "deadlines": [ { "deadline_tag": "read_inputs", "range": { "min_ms": 1, "max_ms": 5 } }, { "deadline_tag": "publish_outputs", "range": { "min_ms": 2, "max_ms": 10 } } ] } ], "heartbeat_monitors": [ { "monitor_tag": "planner_heartbeat", "range": { "min_ms": 90, "max_ms": 110 } } ], "logic_monitors": [ { "monitor_tag": "operation_mode_flow", "initial_state": "initializing", "states": [ { "state": "initializing", "allowed_transitions": ["running", "fault"] }, { "state": "running", "allowed_transitions": ["degraded", "fault"] }, { "state": "degraded", "allowed_transitions": ["running", "fault"] }, { "state": "fault", "allowed_transitions": [] } ] } ] }