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

Minor inconcistencies in METRIC_XXX tags #253

Open
llucax opened this issue Sep 16, 2024 · 1 comment
Open

Minor inconcistencies in METRIC_XXX tags #253

llucax opened this issue Sep 16, 2024 · 1 comment
Labels
part:protobuf Affects the protocol buffer definition files type:enhancement New feature or enhancement visitble to users
Milestone

Comments

@llucax
Copy link
Contributor

llucax commented Sep 16, 2024

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.

@llucax llucax added type:enhancement New feature or enhancement visitble to users part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed labels Sep 16, 2024
@llucax llucax changed the title Gap between METRIC_AC_REACTIVE_ENERGY_PHASE_1 and METRIC_AC_REACTIVE_ENERGY_PHASE_2 Minor inconcistencies in METRIC_XXX tags Sep 16, 2024
@llucax llucax added part:protobuf Affects the protocol buffer definition files and removed part:❓ We need to figure out which part is affected priority:❓ We need to figure out how soon this should be addressed labels Sep 17, 2024
@tiyash-basu-frequenz
Copy link
Contributor

tiyash-basu-frequenz commented Sep 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:protobuf Affects the protocol buffer definition files type:enhancement New feature or enhancement visitble to users
Projects
None yet
Development

No branches or pull requests

2 participants