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

consider splitting namespaces for requested settings and actual settings #81

Open
jordens opened this issue Feb 10, 2022 · 7 comments
Open
Labels
enhancement New feature or request

Comments

@jordens
Copy link
Member

jordens commented Feb 10, 2022

@ryan-summers
Copy link
Member

I think we would need to see an actual use case for this before moving forward. Right now, the only benefit I see is in reporting quanitzed values.

quartiq/booster#206 was found to be caused by #71, so it doesn't quite fall under this, and I believe we came to the conclusion that a retained setting should apply if the MQTT client is reconnected.

I think the main concern that this would address is when a settings configuration has a "side effect". In this case, some requested setting would need to be significantly changed as a result of another. However, as of yet, we haven't seen that use case arise.

@jordens
Copy link
Member Author

jordens commented Feb 16, 2022

One general argument is that the device must in general republish actual settings, not just on startup.

  • As soon as there are multiple ways that settings can change, e.g. and especially if those happen independent of MQTT (EEPROM, buttons, external state changes) the MQTT world wants to know about the actual state.
  • If there is quantization or
  • If there are bounds that are applied when converting a desired setting to an actual one
  • If changing setting A has sideeffects, e.g. setting B mus then be change to respect bounds. (transformation changes on booster affecting the interlock limits)
  • If we have an expensive settings change where application takes a while, this provides the natural response topic to acknowledge/publish the result, the log topic is hard to handle in the API and likely to be forgotten since there is no consequence of the messages on it.
  • Complicated state machine events: state machine is in A, user requests B on MQTT, state machine transitions to C, then event D comes in through I2C temperature reading, state machine transitions to D.

The Device can not republish to the same topic:

  • there can only be one retained message per topic (I assume without having checked). If it were to republish to the same topic, we remove and replace the retained message (not desirable) and either make the new one retained (not desirable either since it may not reflect the desired state on the next boot) or non-retained (thus loosing all info).
  • We also can't re-publish to the same topic as that creates transient states that don't correspond to reality.
  • Settings requests and actual settings are semantically different and should be separate.

Summarizing, the device:

  • should always republish its actual state (to signal actual state, reasons above)
  • be authoritative in doing that (to avoid transient unphysical info)
  • Settings republication should not be retained (they are not applicable when the device is e.g. off)
  • It should be separate from telemetry. Telemetry is periodic, settings republication is event-triggered, both may well see very different handling in downstream logging tools, influxdb/grafana

@ryan-summers
Copy link
Member

ryan-summers commented Feb 16, 2022

It should be separate from telemetry. Telemetry is periodic, settings republication is event-triggered, both may well see very different handling in downstream logging tools, influxdb/grafana

This is the key point that I was missing and had forgotten since our last chat. All of the above information can be shovelled off to telemetry without issue, but it's a good point that that's not necessarily the desired result e.g. because of logging infra.

The main issue I'm trying to reconcile is that, at least for Booster, the requested settings and the actual state are actually two different structures. For booster, the ChannelState variable is not actually requestable via Miniconf (and doesn't impl Miniconf), but rather incoming settings cause transitions of the ChannelState. Thus, any "actual" settings of the device will have a different format than the original Settings: Miniconf struct

@jordens
Copy link
Member Author

jordens commented Feb 16, 2022

Ack. I think it's fine and desirable that requested settings and actual state (let's call it that) are not the same struct and may not be the same values either (see the statemachine "event" vs "state" case).

@jordens
Copy link
Member Author

jordens commented Feb 16, 2022

The one potentially disappointing effect of this may be that now a new settings request coming in, should never be applied using the previous requested settings as context, but always and only the current state as context. That may make the entire idea of having one Miniconf struct obsolete.

@ryan-summers
Copy link
Member

@jordens and I discussed this today at length and came to the following conclusions:

  1. We should explicitly exclude potential side-effects of settings. For booster, as an example, it was found that modification of the interlock transform and the interlock level can independently result in impossible hardware configurations. The proposed solution for this was to refactor the setting so that both the transform + level had to be modified atomically. This ensures that there's a single setting to configure a single hardware feature
    • Main point: Settings may not have side-effects
  2. For Booster, we want to to log various state information as it changes, and not necessarily periodically (e.g. record when a channel disables, re-enables, etc.) Additionally, on Booster, configuration is possible via both the front panel buttons and the Miniconf API. Pressing the front panel button does not affect requested configurations, but does change hardware state. Such an example could not be identified in Stabilizer, the other current user of Miniconf, and it was found that this may be a potential niche use case.

As a workaround to (2), it was proposed that we should actually implement this type of state tracking + republication of current state at the Booster application-level logic. In the scenario, Booster's telemetry client would be used for publishing channel state changes (and potentially any other settings state values) to a separate topic namespace in MQTT. This would let us tailor the implementation to Booster for now, and if we see a more generic need for this type of API, we could implement it into Miniconf or some other library in a generic fashion.

Summary:

  • Leave Miniconf as implemented currently
  • Add specific code to the Booster application for reporting current state on a separate topic tree. Booster's application logic is used to determine when to publish updates.

@ryan-summers
Copy link
Member

Upon thinking about more Miniconf use cases, it occurs to me that there's another abstract desire for requested vs. actual settings.

If there's an optional setting that is present during one execution, the user may configure it as a Retained setting value. In this case, it will continue to be republished whenever someone subscribes.

If the device is then reconnected later such that the setting is no longer present, it may appear as if it is present because of the requested configuration.

An example of this would be configuring an RF channel on Booster. If the user configured a channel as a retained setting, but then removed that channel and reboot booster, it would not be obvious from the available topics that the channel is not actually present.

@jordens jordens added the enhancement New feature or request label Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants