frequenz-floss / frequenz-api-common

Shared protobuf definitions and Python bindings for Frequenz APIs
https://frequenz-floss.github.io/frequenz-api-common/
MIT License
1 stars 12 forks source link

Minor inconcistencies in `METRIC_XXX` tags #253

Open llucax opened 2 months ago

llucax commented 2 months ago

What's needed?

Not sure if we want to fix this, but there a (probably unintentional) jump from 67 to 69 in METRIC_AC_REACTIVE_PHASE_1 to 2:

  METRIC_AC_REACTIVE_ENERGY = 66;
  METRIC_AC_REACTIVE_ENERGY_PHASE_1 = 67;
  METRIC_AC_REACTIVE_ENERGY_PHASE_2 = 69;
  METRIC_AC_REACTIVE_ENERGY_PHASE_3 = 70;

Also METRIC_BATTERY_CAPACITY starts at 101:

  // General BMS metrics.
  METRIC_BATTERY_CAPACITY = 101;
  METRIC_BATTERY_SOC_PCT = 102;
  METRIC_BATTERY_TEMPERATURE = 103;

And General sensor metrics has a jump too:

  // General sensor metrics
  METRIC_SENSOR_WIND_SPEED = 160;
  METRIC_SENSOR_WIND_DIRECTION = 162;
  METRIC_SENSOR_TEMPERATURE = 163;
  METRIC_SENSOR_RELATIVE_HUMIDITY = 164;
  METRIC_SENSOR_DEW_POINT = 165;
  METRIC_SENSOR_AIR_PRESSURE = 166;
  METRIC_SENSOR_IRRADIANCE = 167;

Proposed solution

Maybe make it contiguous?

  METRIC_AC_REACTIVE_ENERGY = 66;
  METRIC_AC_REACTIVE_ENERGY_PHASE_1 = 67;
  METRIC_AC_REACTIVE_ENERGY_PHASE_2 = 68;
  METRIC_AC_REACTIVE_ENERGY_PHASE_3 = 69;

Luckily there is only these 2 tags to fix, as the next enum value has a (logical) to tag 80 for Ac harmonics.

And reset to 100?

  // General BMS metrics.
  METRIC_BATTERY_CAPACITY = 100;
  METRIC_BATTERY_SOC_PCT = 101;
  METRIC_BATTERY_TEMPERATURE = 102;

Also we need to fix only 2 tags, as next enum value has a (logical) to tag 120 for General inverter metrics.

And make it contiguous too:

  // General sensor metrics
  METRIC_SENSOR_WIND_SPEED = 160;
  METRIC_SENSOR_WIND_DIRECTION = 161;
  METRIC_SENSOR_TEMPERATURE = 162;
  METRIC_SENSOR_RELATIVE_HUMIDITY = 163;
  METRIC_SENSOR_DEW_POINT = 164;
  METRIC_SENSOR_AIR_PRESSURE = 165;
  METRIC_SENSOR_IRRADIANCE = 166;

We have to fix a few more here, but it is also not too bad, these are the last items in the enum.

tiyash-basu-frequenz commented 2 months ago

Sure, we can take these in the next breaking change release.