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

[BMP280] Fix regression on #636 + split overlap in code between C001 and C002 into separate function #890

Closed
TD-er opened this issue Feb 17, 2018 · 5 comments
Labels
Type: Bug Considered a bug
Milestone

Comments

@TD-er
Copy link
Member

TD-er commented Feb 17, 2018

See topic: http://www.letscontrolit.com/forum/viewtopic.php?f=18&t=4864
Was fixed in #636

Bug was re-introduced in: 3922827

So @psy0rz if there was a good reason for this change, please have a look at it.
Or else, I will create a PR to fix it again.

@TD-er
Copy link
Member Author

TD-er commented Feb 17, 2018

Hmmm I think there is just too much overlap between C001 and C002.
The differences are probably bugs fixed in one and not the other.

The last sentence of this paragraph suggests the order of the values should be the same for HTTP and MQTT: https://www.domoticz.com/wiki/MQTT#Domoticz_to_MQTT
And there are more examples in which they are not the same.

@TD-er TD-er changed the title [BMP280] Fix regression on #636 [BMP280] Fix regression on #636 + split overlap in code between C001 and C002 into separate function Feb 17, 2018
TD-er added a commit to TD-er/ESPEasy that referenced this issue Feb 17, 2018
… to helper

Both Domoticz controllers had quite some overlap in code.
And where they differ, one of them was faulty and/or already fixed.

Now this overlap is split into a Domoticz helper file to keep code duplication to a minimum.
@TD-er
Copy link
Member Author

TD-er commented Feb 17, 2018

Added a pull request #891
Please test these with as many sensors as possible.
At least it should not matter anymore if HTTP or MQTT Domoticz controller is used.

TD-er added a commit to TD-er/ESPEasy that referenced this issue Feb 18, 2018
Do not store references in a Json object, but just the string :)
psy0rz pushed a commit that referenced this issue Feb 18, 2018
)

* [issue #890] Split overlap for both Domoticz controllers to helper

Both Domoticz controllers had quite some overlap in code.
And where they differ, one of them was faulty and/or already fixed.

Now this overlap is split into a Domoticz helper file to keep code duplication to a minimum.

* [issue #890] Fix some wierd data corruption

Do not store references in a Json object, but just the string :)

* [Domoticz] Add WiFi strength and battery info
@TD-er TD-er added the Type: Bug Considered a bug label Feb 19, 2018
@TD-er TD-er added this to the 2.0.0 milestone Feb 19, 2018
@psy0rz
Copy link
Member

psy0rz commented Feb 20, 2018

yeah there where indeed some strange differences. ill run the controller tests now, it will create dummy for all datatypes, so it will fully test domoticz http and mqtt.

@psy0rz
Copy link
Member

psy0rz commented Feb 20, 2018

you either fixed or broke SENSOR_TYPE_TEMP_BARO. it seems your implementation has actually comments and links to the domoticz api, like it should, so i'll go with yours and fix the regression test accordingly. :)

@psy0rz
Copy link
Member

psy0rz commented Feb 20, 2018

INFO:esptest:*** All tests completed ***

@psy0rz psy0rz closed this as completed Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

2 participants