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

hlw8012: fix constants for BL0937 #1973

Merged
merged 4 commits into from Jul 4, 2021
Merged

hlw8012: fix constants for BL0937 #1973

merged 4 commits into from Jul 4, 2021

Conversation

ianchi
Copy link
Contributor

@ianchi ianchi commented Jun 30, 2021

With the current implementation it is impossible to calibrate all measurements at the same time for the BL0937 sensor as it has different constants factors for converting pulses.

If you set voltage_divider and current_resistor to correctly calibrate voltage and current, you get a reading for power that is completely off.

Add a configuration option sensor_model to handle constants on per sensor model basis.

While at it, avoid computing the same factor on every update and move to setup logic to do it only once.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes BL0937 sensor readings

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#1282

Test Environment

  • ESP32
  • ESP8266

Example entry for config.yaml:

sensor:
  - platform: hlw8012
    sel_pin:
      number: GPIO12
      inverted: true
    cf_pin: GPIO4
    cf1_pin: GPIO5
    sensor_model: BL0937
    voltage_divider: 1685

    current:
      name: Current
    voltage:
      name: Voltage
    power:
      name: Power

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

I cant help but think there is a reason for the / 1000000.0f in the final calculations. Perhaps precision is lost if the multiplier is a smaller number/more decimals before multiplying the cf_hz/cf1_hz?
(If I am spitting out garbage, just let me know.)

The only other comment I have is probably, use the existing CONF_MODEL. You don't need to make a new one and call everything ...SensorModel when Model will do fine.

@OttoWinter
Copy link
Member

@jesserockz That depends a bit on the scale of those values, but iirc those constants were just there for easier code (because iirc the datasheet uses those units). If anything, I think it should be more accurate now, because IEEE754 floats have higher precision closer to 0.

The BL0937 sensor has different constants factors for converting pulses.
Add a configuration to handle constants on per sensor model basis.
While at it, avoid computing the same factor on every update
and move to setup logic to do it only once.
@ianchi
Copy link
Contributor Author

ianchi commented Jul 1, 2021

Fixed name of config option to model.
Changed docs PR accordingly.

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Just 2 suggestions below to use the cv.enum helper

esphome/components/hlw8012/sensor.py Outdated Show resolved Hide resolved
esphome/components/hlw8012/sensor.py Outdated Show resolved Hide resolved
esphome/const.py Outdated Show resolved Hide resolved
@probot-esphome probot-esphome bot removed the core label Jul 4, 2021
ESPHome Dev automation moved this from Needs Review to Reviewer Approved Jul 4, 2021
@jesserockz jesserockz merged commit d31040f into esphome:dev Jul 4, 2021
ESPHome Dev automation moved this from Reviewer Approved to Done Jul 4, 2021
This was referenced Jul 14, 2021
This was referenced Jul 21, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
@ianchi ianchi deleted the hlw8012 branch February 17, 2022 21:27
ESPHome Dev automation moved this from Done to Reviewer Approved Sep 23, 2023
ESPHome Dev automation moved this from Reviewer Approved to Done Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants