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

Added UPS battery remaining time #1292

Closed
wants to merge 5 commits into from

Conversation

briancjohns
Copy link
Contributor

No description provided.

@cstirdivant cstirdivant self-requested a review March 7, 2024 21:38
@@ -83,3 +83,17 @@ UPS_SS_UPSBM:
- UPS
- SS
- UPSBM

UPS_UPSBM:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You cannot name this canonical type using an abstract type you aren't implementing. You need to fully implement UPSBM instead. We want to avoid defining individual fields on a canonical type.

Based on the set of fields you have here, it seems like you'd want to implement UPSBM and IOBM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_canonical: false

I don't want to implement IOBM because it contains quite a few required fields that we don't have. UPSBM requires "remaining_charge_sensor", which we also don't have. I've created a field called "remaining_battery_time_sensor" that accurately reflects the data we do have.

Copy link
Contributor Author

@briancjohns briancjohns Mar 11, 2024

Choose a reason for hiding this comment

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

I've changed the type name to a non-canonical type name UPS_UPSTR

tools/tooling/Scripts/python.exe Outdated Show resolved Hide resolved
@@ -2915,6 +2915,9 @@ literals:
- LOW
- MEDIUM
- HIGH
- remaining_battery_time_sensor:
Copy link
Collaborator

Choose a reason for hiding this comment

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

use existing field remaining_charge_time_sensor instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does "remaining_charge_time_sensor" indicate the amount of time remaining until the battery is fully charged, or the amount of time remaining until the battery is fully discharged? I took it to mean the former, which leaves an important value out of the ontology, being the amount of time the UPS will continue to power the given load in battery mode.

@cstirdivant
Copy link
Collaborator

cstirdivant commented Mar 12, 2024

Hi @briancjohns I cannot review this until it only contains yaml files within the ontology/ folder. Please ensure you are committing from within the ontology/ folder

@briancjohns briancjohns deleted the UPSAddBattTime branch March 13, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants