Skip to content

Commit

Permalink
Deprecate scaled value
Browse files Browse the repository at this point in the history
  • Loading branch information
@nurikk
nurikk committed Jan 12, 2021
1 parent 0f093ea commit 5f46863
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 11 deletions.
2 changes: 1 addition & 1 deletion CC2530DB/GenericApp.ewp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1420,7 +1420,7 @@
</option>
<option>
<name>OGChipConfigPath</name>
<state>$TOOLKIT_DIR$\config\devices\Texas Instruments\CC2530F256.i51</state>
<state>C:\Program Files (x86)\IAR Systems\Embedded Workbench 8.1\8051\bin\..\config\devices\Texas Instruments\CC25xx\3x\CC2530F256.i51</state>
</option>
</data>
</settings>
Expand Down
3 changes: 1 addition & 2 deletions Source/zcl_app.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,7 @@ static uint8 zclApp_ReadBME280(struct bme280_dev *dev) {
if (rslt == BME280_OK) {
LREP("ReadBME280 t=%ld, p=%ld, h=%ld\r\n", bme_results.temperature, bme_results.pressure, bme_results.humidity);
zclApp_Sensors.BME280_HumiditySensor_MeasuredValue = (uint16)(bme_results.humidity * 100 / 1024.0) + zclApp_Config.HumidityOffset;
zclApp_Sensors.BME280_PressureSensor_ScaledValue = bme_results.pressure / 10.0 + zclApp_Config.PressureOffset; // FYI mmhg = bme_results.pressure/133.322
zclApp_Sensors.BME280_PressureSensor_MeasuredValue = zclApp_Sensors.BME280_PressureSensor_ScaledValue / 10.0;
zclApp_Sensors.BME280_PressureSensor_MeasuredValue = bme_results.pressure / 100.0 + zclApp_Config.PressureOffset / 100.0;
zclApp_Sensors.Temperature = (int16)bme_results.temperature + zclApp_Config.TemperatureOffset;
} else {
LREP("ReadBME280 bme280_get_sensor_data error %d\r\n", rslt);
Expand Down
2 changes: 0 additions & 2 deletions Source/zcl_app.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ typedef struct {
int16 BME280_Temperature_Sensor_MeasuredValue;
uint16 BME280_HumiditySensor_MeasuredValue;
int16 BME280_PressureSensor_MeasuredValue;
int16 BME280_PressureSensor_ScaledValue;
int8 BME280_PressureSensor_Scale;
} sensors_state_t;

/*********************************************************************
Expand Down
8 changes: 2 additions & 6 deletions Source/zcl_app_data.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const uint16 zclApp_clusterRevision_all = 0x0002;
// Basic Cluster
const uint8 zclApp_HWRevision = APP_HWVERSION;
const uint8 zclApp_ZCLVersion = APP_ZCLVERSION;
const uint8 zclApp_ApplicationVersion = 3;
const uint8 zclApp_ApplicationVersion = 4;
const uint8 zclApp_StackVersion = 4;

//{lenght, 'd', 'a', 't', 'a'}
Expand Down Expand Up @@ -78,9 +78,7 @@ sensors_state_t zclApp_Sensors = {
.CO2_PPM = 0,
.Temperature = 0,
.BME280_PressureSensor_MeasuredValue = 0,
.BME280_HumiditySensor_MeasuredValue = 0,
.BME280_PressureSensor_ScaledValue = 0,
.BME280_PressureSensor_Scale = -1
.BME280_HumiditySensor_MeasuredValue = 0
};

/*********************************************************************
Expand All @@ -103,8 +101,6 @@ CONST zclAttrRec_t zclApp_AttrsFirstEP[] = {
{TEMP, {ATTRID_TemperatureOffset, ZCL_INT16, RW, (void *)&zclApp_Config.TemperatureOffset}},

{PRESSURE, {ATTRID_MS_PRESSURE_MEASUREMENT_MEASURED_VALUE, ZCL_INT16, RR, (void *)&zclApp_Sensors.BME280_PressureSensor_MeasuredValue}},
{PRESSURE, {ATTRID_MS_PRESSURE_MEASUREMENT_SCALED_VALUE, ZCL_INT16, RR, (void *)&zclApp_Sensors.BME280_PressureSensor_ScaledValue}},
{PRESSURE, {ATTRID_MS_PRESSURE_MEASUREMENT_SCALE, ZCL_INT8, RR, (void *)&zclApp_Sensors.BME280_PressureSensor_Scale}},
{PRESSURE, {ATTRID_PressureOffset, ZCL_INT32, RW, (void *)&zclApp_Config.PressureOffset}},


Expand Down

12 comments on commit 5f46863

@dmitrychepel
Copy link
Contributor

@dmitrychepel dmitrychepel commented on 5f46863 Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

pls explain why do you remove ATTRID_MS_PRESSURE_MEASUREMENT_SCALED_VALUE. Now it worked correct, and it is necessary for sls gateway for example.
we dealed with other that it will. if you have an issue please report it.

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 12, 2021

Choose a reason for hiding this comment

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

pls explain why do you remove ATTRID_MS_PRESSURE_MEASUREMENT_SCALED_VALUE. Now it worked correct, and it is necessary for sls gateway for example.
we dealed with other that it will. if you have an issue please report it.

This feature didn't work as expected, caused many troubles. This is why I've deprecated it.

@dmitrychepel
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls provide some infornation. Now I see here https://github.com/presslab-us/ZigBee_SensorTag2 for example has the same implementation. And I tested it in my environment in z2m where scale is used, and in SLS, where it required. Nobody explain even one bug with scaled. But It may be overflow on pow. I removed it, and mean that scale always equal -1. If error will appeared I will research it and will fix.

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 13, 2021

Choose a reason for hiding this comment

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

The main issue is that scaled value is always equals to measured value

@dmitrychepel
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not issue, but it is not the same.
there are heave some differences.
lets see
zclApp_Sensors.BME280_PressureSensor_ScaledValue = bme_results.pressure / 10.0 ...
zclApp_Sensors.BME280_PressureSensor_MeasuredValue = zclApp_Sensors.BME280_PressureSensor_ScaledValue / 10.0;
so scaled greater than not scaled in 10 times.

And
{PRESSURE, {ATTRID_MS_PRESSURE_MEASUREMENT_SCALED_VALUE, ZCL_INT16, ...
{PRESSURE, {ATTRID_MS_PRESSURE_MEASUREMENT_MEASURED_VALUE, ZCL_INT16,
so
scaled pressure looks like 10184
measured 1018
in z2m if scaled present
result = scaled/scale = 1018,4
if not
result = measured = 1018.0

so fraction part is lost.
for instance, if I will do additional operations convert into mmhg if will have lose last digit.
it will happend after two rounds. so I will have additional +-0,5 to mmhg accuracy.
in case of using scaled value only +-0,05.
1018,4 / 133 = 765,7 = 766
1018,0 / 133 = 765,4 = 765

The second, some devices such as sls gateway required scaled value for pressure. it does not show pressure. we check it together with sls gateway author.
for me it is not important, I using sls only for check my devices, but when I show my version, i receive feedbacks that sls gateway pressure become to be broken.

The third, other project use scaled. and it is fit to specification of domains zigbee.
Pressure has scaled value, but some others not.
I think that in this case much more convenient to be fit to specification and ask others to fit within specification too.

the forth, I ready to spent time to fix issues related to pressure.
so, please, lets returns scaled values.

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 13, 2021

Choose a reason for hiding this comment

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

Nothing wrong with using the measured value, it's 100% according to zcl6 spec

@dmitrychepel
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but noting wrong by using scaled too.

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 13, 2021

Choose a reason for hiding this comment

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

Yes, but noting wrong by using scaled too.

Scaled value proved it's self as buggy stuff, I've wasted enough time on this. Fraction of precision doesn't worth more of time

@dmitrychepel
Copy link
Contributor

Choose a reason for hiding this comment

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

I ready to spent time to fix such bugs. do you have bugs artifacts?
I see that in my version in z2m pressure is stable for a long time.
in sls i see correct value, but not check it for a long time, but without scaled, sls is not show pressure.

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 13, 2021

Choose a reason for hiding this comment

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

Let sls developer fix it's code and settle down with this. Even if you'll spend time on fixing, I would have to validate the changes, which would consume my time.

@dmitrychepel
Copy link
Contributor

Choose a reason for hiding this comment

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

do you have plans to remove support of scaled measure from z2m converter in the z2m codebase?

@nurikk
Copy link
Contributor Author

@nurikk nurikk commented on 5f46863 Jan 13, 2021

Choose a reason for hiding this comment

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

do you have plans to remove support of scaled measure from z2m converter in the z2m codebase?

Just replace scaled value with measured by reporting configuration from converter, everything else shall remain intact. I've already raised PR

Please sign in to comment.