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

Fix duplication temperature/device_temperature SSM-U01 #4267

Merged
merged 1 commit into from
May 21, 2022

Conversation

McGiverGim
Copy link
Contributor

The SSM-U01 now is publishing two sensors (temperature and device_temperature) that get the data from the same "physical" sensor. So it has a duplication in the sensors published.

image

The SSM-U01 does not publish the temperature (cluster msTemperatureMeasurement), in place it publishes the device_temperature (cluster genDeviceTempCfg). It publishes the same value too in the aqaraOpple cluster, attribute 3.

This PR removes the temperature converter from the device, letting only the device_temperature one, and modifies the aqara_opple converter to publish the value as device_temperature in place of temperature.

I'm pretty sure some people will complain about this, because the device_temperature has no options to "calibrate" it. But we need to unify both sensors, in one sense or in the other.

@Otnow
Copy link
Contributor

Otnow commented May 20, 2022

I think that it is necessary to rename the value of attribute 3 for all from temperature to device_temperature or chip_temperature, because this is the service value of the chip temperature.

@McGiverGim
Copy link
Contributor Author

@Otnow maybe is the correct move, looking at debug logs from the WSDCGQ11LM and the WSDCGQ01LM nobody of them publish the the "3" attribute.

My only doubt is about the exceptions:

if (['WSDCGQ12LM'].includes(model.model)) {
// This temperature value is ignored because with greater accuracy it is reported in key №100
} else if (!['WXCJKG11LM', 'WXCJKG12LM', 'WXCJKG13LM'].includes(model.model)) {
payload.temperature = calibrateAndPrecisionRoundOptions(value, options, 'temperature'); // 0x03
}

By the comment, I suppose the WSDCGQ12LM publishes the "3" value, but maybe is not too bad to publish it without less precision as device_temperature and continue publishing with high precision as temperature.

The other devices I suppose they publish the "3" attribute but it is not the temperature.

@Koenkk what do you think? Maybe simply change the temperature by device_temperature in the "3" attribute? Remove the exception for WSDCGQ12LM or let it?

@Otnow
Copy link
Contributor

Otnow commented May 20, 2022

if (['WSDCGQ12LM'].includes(model.model)) {
// This temperature value is ignored because with greater accuracy it is reported in key №100

I added this exception, so if we rename temperature to device_temperature, then this condition can be removed, because in this case, there will be no intersection of the value with temperature from attribute 100 for WSDCGQ12LM.

@McGiverGim
Copy link
Contributor Author

Ok, perfect! Let's wait for @Koenkk revision to see if he is aligned with the change. I will search too for other devices with gen_basic or aqara_opple that expose temperature. Maybe some of them must start to expose device_temperature.

@Koenkk
Copy link
Owner

Koenkk commented May 21, 2022

Looks good, thanks!

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

Successfully merging this pull request may close these issues.

3 participants