Skip to content

Conversation

@nhoening
Copy link
Contributor

This is useful in general, but especially for getting started and then seeing your power sensors on the dashboard.

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening requested a review from Flix6x January 24, 2022 17:45
@nhoening nhoening added this to the 0.8.0 milestone Jan 24, 2022
@nhoening nhoening added the CLI label Jan 24, 2022
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Honestly, I don't particularly like adding this custom logic to support a single attribute, also because it's an attribute that we intend to replace by a sensor of itself. I have an alternative suggestion:

@click.option(
    "--attributes",
    required=False,
    type=str,  # a json string, actually (maybe there is out-of-the-box validation for that)
    help="Additional attributes. Hint: for sensors that measures power, use {"capacity_in_mw": 10} to set a capacity of 10 MW",
    default = "{}",
)

@nhoening
Copy link
Contributor Author

We have logic relying on that attribute being there, so it's not weird to also have logic to make sure the power sensors have it when being made.
Even if we support all potential attributes to be passed in with JSON (for which I haven't found any click support), I'd still want that check.
That code which relies on the attribute and this new code are both to-be deprecated, I think that's fine.

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening nhoening requested a review from Flix6x January 24, 2022 21:58
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Okay, I like this better, and just as a placeholder. I'd like us to follow up with a ticket to replace any logic using the capacity_in_mw attribute, such that it becomes a completely optional attribute. I believe it's at least used to set the scale range on the latest state plot, and by schedulers to set constraints on the power flow. For the former we can use a max on the history, or let it go altogether (let the chart determine the scale range), and for the latter we can let constraints be set through a dedicated UDI event field (an optional field, in case the sensor attribute is set as a fallback).

@nhoening
Copy link
Contributor Author

nhoening commented Jan 25, 2022

I agree. I'll make the ticket.

As a slightly longer outlook: For the former (and the latter maybe as well) we can also first look for a dedicated capacity sensor, then fall back to something more simple and robust.

In 2022, I think we need to make sure the new data model is super straightforward to use, as well as robust.

@nhoening nhoening merged commit 280de45 into main Jan 25, 2022
@nhoening nhoening deleted the cli-add-sensors-with-capacity-if-they-measure-power branch January 25, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants