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

Add QMC5883L Sensor + Improvements to HMC5883L #671

Merged
merged 21 commits into from
Nov 26, 2019
Merged

Add QMC5883L Sensor + Improvements to HMC5883L #671

merged 21 commits into from
Nov 26, 2019

Conversation

timpur
Copy link
Contributor

@timpur timpur commented Jul 21, 2019

Description:

Related issue (if applicable): fixes

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

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:

Added QMC5883L sensor and improvements to the HMC5883L sensor

Docs: esphome/esphome-docs#301

@timpur
Copy link
Contributor Author

timpur commented Jul 22, 2019

getting this issue in build checks, not my code ??

-    msg.effects.push_back("None");
+    msg.effects.emplace_back("None");

think this is related to #667 @OttoWinter ...

HMC5883LDatarates, unit="Hz", int=False),
cv.Optional(CONF_MEASUREMENT_MODE, default='normal'): validate_enum(
HMC5883LMeasurementModes, int=False),
cv.Optional(CONF_RANGE, default='130uT'): validate_enum(HMC5883L_RANGES, unit="uT"),
Copy link
Member

Choose a reason for hiding this comment

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

This should accept both µT and uT (otherwise this would be a breaking change).

return;
}

// if (id != 0x01) {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove commented-out code or add comments for future use.

}

bool QMC5883LComponent::read_byte_16_(uint8_t a_register, uint16_t *data, uint32_t conversion) {
bool success = QMC5883LComponent::I2CDevice::read_byte_16(a_register, data, conversion);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool success = QMC5883LComponent::I2CDevice::read_byte_16(a_register, data, conversion);
bool success = this->read_byte_16(a_register, data, conversion);

def validate_enum(enum_values, unit=None, int=True):
def validate_enum_bound(value):
value = cv.string(value)
if unit and (value.endswith(unit.encode(encoding='UTF-8', errors='strict'))
Copy link
Member

Choose a reason for hiding this comment

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

unit.encode should not be necessary here - value is already a unicode string (py2: unicode, py3: str)

@timpur
Copy link
Contributor Author

timpur commented Sep 10, 2019

Thanks @OttoWinter, will try get this done this weekend.

@timpur
Copy link
Contributor Author

timpur commented Oct 16, 2019

@OttoWinter Believe this is ready fir review again thx :)

Timothy Purchas and others added 12 commits October 16, 2019 21:53
Either the units are converted to the user values like 1x, 8x oversampling or not printed at all. Printing the machine-value of these is only confusing users.
Move stuff that can be done beforehand out of the bound function, use text_type for py2/3 compatability.
@timpur
Copy link
Contributor Author

timpur commented Oct 29, 2019

@OttoWinter Did you want to clean up now or later ?

@OttoWinter
Copy link
Member

@timpur What do you mean?

@pilotak
Copy link

pilotak commented Nov 24, 2019

I have checked QMC on ESP8266 and also on ESP32, it works like a charm.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants