Documentation improvements ideas

[llucax]

I will use this discussion to make annotations while I read the documentation to implement the microgrid API client, with doubts or things I think are wrong or confusing, with the hopes we can use it to improve the docs.

Action points

To know what each number is about, please see the Issues section.

• 1, 2: Create an issue or PR to change names to be more consistent (@tiyash-basu-frequenz)
• 3: Create PR to update the docs to say it is unspecified, not unknown (@llucax)
• 4: Think about if we remove the GridConnectionPoint field and instead require to have a Fuse in the component graph (@tiyash-basu-frequenz)
• 5: Names stay as they are but create an issue or PR to improve the docs to mention the ratios and make it clear that primary and secondary are not actually voltages but number of coil turns or something like that (@tiyash-basu-frequenz)
• 6: Create a meeting to discuss how lifetime is supposed to be used to decide how to specify it correctly (@tiyash-basu-frequenz)
• 7: Remove restriction from the client. ID can be 0 for any component category and can't be negative because it is a uint64 (@llucax)

Issues

1. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/components/components.proto#L134C3-L134C59: Component.category_type docs say "The metadata specific to the component category type.", which looks pretty odd. See Component Category Metadata/Type for more details.
2. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/components/components.proto#L90: The name of the message also looks a bit odd, at least the name of the field and the name of the message don't look consistent. See Component Category Metadata/Type for more details.
3. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/components/components.proto#L27-L28: Typo? "An unknown component categories" (-> "category"). Also the description sounds odd. Shouldn't this be only set in a bad request, or when the data is missing in the PB message somehow? Shouldn't this value be taken generally as an error?
4. We have a new Fuse message, but the GridConnectionPoint message is still using a unint32 rated_fuse_current field. Should it have a Fuse instead?
5. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/components/transformer.proto#L19-L28: It is hard to reason about a voltage transformer primary and secondary voltages. If this is a common term in the industry then OK, otherwise I recommend changing to input_voltage and output_voltage. Also although it might be obvious, it could be worth specifying the units (volts?) for those fields.
6. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/lifetime.proto#L22-L28: It might be good to document if the fields are required or optional and what does it mean if any is missing. I will assume both are optional and the lack of start means it was there forever and the lack of end means the asset is still there and there is no plan to remove it (if the whole message is missing, I will also consider that the asset is there forever). I also wonder what does this mean for the microgrid API in particular, will it ever return assets that are already removed? Maybe it is worth documenting in https://github.com/frequenz-floss/frequenz-api-microgrid/blob/6291e8e9d1c0769ec7b93cb8300dfb3c3f096ec0/proto/frequenz/api/microgrid/v1/microgrid.proto#L38-L63.
7. https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/components/components.proto#L123: Are there any restrictions on component IDs (and microgrid IDs and enterprise IDs while we are at it)? In the current microgrid client code there is a validation to check that component IDs are >= 0 and there is an error if the component ID 0 is not a GRID component. Does this still holds true?

Ideas

[1, 2] Component Category Metadata/Type

For one side I feel a bit like the ComponentCategory is redundant with ComponentCategoryMetadataVariant, if we would define one message for every category, then it is enough with ComponentCategoryMetadataVariant, the oneof will tell us which category we are dealing with.

But I assume the ComponentCategory is used much more widely, so we need to keep it for other purposes, so let's keep looking for ideas assuming this.

If we forget about the category, it seems like what we have here is the category-specific characteristics from a component. What about naming ComponentCategoryMetadataVariant something like CategorySpecificInfo. For me the name MetadataVariant is super confusing, and the field name category_type is even more confusing.

This is also how it reads when accessing this info:

metadata = component.category_type.WhichOneOf("metadata")
if metadata == "battery":
    battery = component.category_type.battery

For me this looks like an enum, like it is just saying it is a battery, not that we are actually getting information about the battery.

Looking at the problem in reverse, something more intuitive could look like:

category = component.category_specific_info.WhichOneOf("category")
if category == "battery":
    battery_info = component.category_specific_info.battery

// Metadata specific to a microgrid component.
message CategorySpecificInfo {
  oneof category {
    frequenz.api.common.v1.microgrid.components.BatteryInfo battery = 1;
    frequenz.api.common.v1.microgrid.components.EvChargerInfo ev_charger = 2;
    frequenz.api.common.v1.microgrid.components.FuseInfo fuse = 3;
    frequenz.api.common.v1.microgrid.components.GridConnectionPointInfo grid = 4;
    frequenz.api.common.v1.microgrid.components.InverterInfo inverter = 5;
    frequenz.api.common.v1.microgrid.components.VoltageTransformerInfo
      voltage_transformer = 6;
  }
}

// Microgrid electrical component details.
message Component {
  // ...

  // The component category. E.g., Inverter, Battery, etc.
  ComponentCategory category = 4;

  // The metadata specific to the component category type.
  CategorySpecificInfo category_specific_info = 5;

  // ...
}

We could use something different than info, an alternative could be field (CategorySpecificFields, BatteryFields, component.category_specific.fields.battery) or even the old metadata (CategorySpecificMetadata, BatteryMetadata, component.category_specific.metadata.battery). I think I personally prefer info though, it seems more general than fields and less cryptic (or more specific) than metadata.

(to be continued...)

[llucax]

I wanted to close this discussion, so I created #256 for item 5.