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

HP303B Sensor support #8638

Merged
merged 16 commits into from
Jun 12, 2020
Merged

HP303B Sensor support #8638

merged 16 commits into from
Jun 12, 2020

Conversation

rjaakke
Copy link
Contributor

@rjaakke rjaakke commented Jun 7, 2020

Description:

Adds support for the HP303B Barometric Pressure Sensor.
I used the Lolin barometric pressure sensor shield.

The default Lolin driver returns an int32 measurement, I modified the driver to return a float for more precise results.

An ESP32 is on its way in the mail, I will test it as soon as it arrives.

Checklist:

  • The pull request is done against the latest dev branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR.
  • The code change is tested and works on core ESP8266 V.2.7.1
  • The code change is tested and works on core ESP32 V.1.12.2
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@device111
Copy link
Contributor

device111 commented Jun 7, 2020

@rjaakke
have a look on some other temp/hum sensors for standarized implementation in tasmota.
(BMP, DHT12, AHTX, HIH6130, a.s.o)
specifically reading functions of every second and initialization of sensor.
thanks

@device111
Copy link
Contributor

@rjaakke
and please use structs for global variables.

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 8, 2020

@rjaakke
and please use structs for global variables.

Resolved

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 8, 2020

@rjaakke
have a look on some other temp/hum sensors for standarized implementation in tasmota.
(BMP, DHT12, AHTX, HIH6130, a.s.o)
specifically reading functions of every second and initialization of sensor.
thanks

Should be correct now, please review

@device111
Copy link
Contributor

your on the right way, but you have killed a lot of code for support of 2 Sensors. At the moment, only one Sensor is detected.
Have a look in the mcp9808 code for init multible I2C adresses.
And if you want, make a correction of the begin() function in the libary, that can give you return a true or false for succesfull initialisation. The init() call in begin() gaves you the return codes for that.

@arendst arendst added the on hold by dev team Result - Feature request put on hold by member of development team label Jun 8, 2020
@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 8, 2020

your on the right way, but you have killed a lot of code for support of 2 Sensors. At the moment, only one Sensor is detected.
Have a look in the mcp9808 code for init multible I2C adresses.
And if you want, make a correction of the begin() function in the libary, that can give you return a true or false for succesfull initialisation. The init() call in begin() gaves you the return codes for that.

Ok, should work on all I2C addresses now, I also modified the LOLIN lib to return a status when calling begin().

if (I2cActive(bhp303b_addresses[i])) { return; }

bhp303b_sensor.address = bhp303b_addresses[i];
if (I2cActive(HP303B_START_ADDRESS + i)) { return; }

Copy link
Contributor

Choose a reason for hiding this comment

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

us continue to stay in the loop and use I2cSetDevice:
if (!I2cSetDevice(HP303B_START_ADDRESS + i )) { continue; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it.

It was unclear to me which one of the two to use as I found both implementations in different sensors.

bhp303b_sensor.type = 1;
hp303b_sensor[hp303b_cfg.count].address = HP303B_START_ADDRESS + i;
I2cSetActiveFound(hp303b_sensor[hp303b_cfg.count].address, hp303b_cfg.types);
hp303b_cfg.count++;
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

strlcpy(sensor_name, hp303b_cfg.types, sizeof(sensor_name));
if (hp303b_cfg.count > 1) {
snprintf_P(sensor_name, sizeof(sensor_name), PSTR("%s%c0x%02X"), sensor_name, IndexSeparator(), hp303b_sensor[i].address); // MCP9808-18, MCP9808-1A etc.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

check all the comments. Some are from MCP9808

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

char str_pressure[33];
dtostrfd(hp303b_sensor[i].pressure, Settings.flag2.pressure_resolution, str_pressure);

if (json)
Copy link
Contributor

Choose a reason for hiding this comment

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

quote from Theo:
domoticz and knx only support one temp sensor index you'l need to add a test for loop index 0 here. See latest change in bh1750.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check and comments added.

Copy link
Contributor

@device111 device111 Jun 9, 2020

Choose a reason for hiding this comment

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

ok, now we have tasmota support of 2 Sensors, but not yet in the libary.

you must change the libary to support reading functions with different I2C addresses.
at the moment, we have only support of one sensor with fix I2c reading functions in the libary.
i.e:
HP303BSensor.measureTempOnce(t, hp303b_cfg.oversampling);
(reads only from the address that was last initialized)

Now, we need a index for the reading functions:
HP303BSensor.measureTempOnce(hp303b_sensor[hp303b_idx].address, t, hp303b_cfg.oversampling);

the local variable for i2c address in the libary is m_slaveAddress
i can help you to change the libary, if you want.

Ps.: good example is the MCP9808 Libary...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna make an attempt but I could use some advice.

Since I'm rewriting the library for Tasmota, do you think I should remove the continuous and interrupt code from the lib to reduce size?

The library also support SPI but I don't know how to test this, is this useful for Tasmota or should remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, you don't must remove the code for SPI or other functions. The compiler compile only used functions, so there is no Advantage and you save no space. you don' must complete re-write the libary.
We need only the 2 little changes and all should work.
But feel free to do it...

you must change only the 2 functions:
measureTempOnce() and
measurePressureOnce()

add a Parameter for I2c-address in both functions and all is working with tasmota.
(note: each function of both is overloaded with 2 different Parameters)

Copy link
Contributor

Choose a reason for hiding this comment

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

example for temp:
int16_t LOLIN_HP303B::measureTempOnce(uint8_t slaveAddress, int32_t &result) { return measureTempOnce(slaveAddress, result, m_tempOsr); }
and
int16_t LOLIN_HP303B::measurePressureOnce(uint8_t slaveAddress; int32_t &result, uint8_t oversamplingRate) { m_slaveAddress = slaveAddress; //start the measurement .......

should work.

don't forget to change the defines in header file...

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, thats bad with the m_initFail and m_opMode
variables.
I have a look tomorrow for it...
I hope, this is not a big Problem..

Copy link
Contributor

Choose a reason for hiding this comment

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

or you make the implementation of more I2c addresses in a second Step.
You can change #define HP303B_MAX_SENSORS 2
to 1 and do first a PR in Tasmota.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, the m_initFail is not a Problem. you can delete the check of this variable from reading function, we have checked the init with the begin() function.
The m_opMode needs a little bit more investigation.

@arendst
can you please check the driver as it is and you can merge it?

With the change #define HP303B_MAX_SENSORS 1, the driver should work with one sensor for now.
the support for two sensors comes later.
thanks in Advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed the change to the lib. It would be strange if you connect 2 sensors and have them running in different operation mode. So this should not be an issue

@device111
Copy link
Contributor

ok, but if you have only one sensor connected, the last m_initFail fail and the var hold the state.
so you must delete the query of m_initFail in the Reading function startMeasureTempOnce()

the libary check in the reading functions if the sensor ist success init but we check this with the begin function and give it back to Tasmota driver.

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 10, 2020

Ok, fixed it.

@device111
Copy link
Contributor

ok, have you tested if all working now?

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 11, 2020

ok, have you tested if all working now?

Yes, the sensor works but I only have one sensor shield to test with.

@device111
Copy link
Contributor

Good Job!
last test: Can you test the sensor with the second I2C address?
If all working, we must wait for Theo, that have a look above the Code and merge the PR.

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 11, 2020

Good Job!
last test: Can you test the sensor with the second I2C address?
If all working, we must wait for Theo, that have a look above the Code and merge the PR.

I tried getting the sensor shield to use the other 0x76 address but for some reason I can't get it to work. I also tried the library examples but with the same result. It seems to be an issue with the shield.

Here's the info:
https://docs.wemos.cc/en/latest/d1_mini_shiled/barometric_pressure.html#arduino

I tried connecting the jumper, tried to connect the jumper to ground with a 4.7k resistor but no luck.

# Conflicts:
#	tasmota/support_features.ino Resolved
#	tools/decode-status.py Resolved
@device111
Copy link
Contributor

i don't know this board, i have seen no jumper on it. have you wired this two Points?:

grafik

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 11, 2020

i don't know this board, i have seen no jumper on it. have you wired this two Points?:

grafik

Yes, I also tried to wire the SDO on the chip directly to GRND but without luck.

@device111
Copy link
Contributor

Have you tried to wire the sensor and make a I2Cscan in Tasmota? Is the 0x76 address found?
If not, something is wrong with the board.
But Driver works now with Tasmota and Theo can check it.

@rjaakke
Copy link
Contributor Author

rjaakke commented Jun 11, 2020

Have you tried to wire the sensor and make a I2Cscan in Tasmota? Is the 0x76 address found?
If not, something is wrong with the board.
But Driver works now with Tasmota and Theo can check it.

Yes, I also tried the basic samples that come with the library and they couldn't find the sensor at 0x76 either.

@arendst arendst merged commit 7919678 into arendst:development Jun 12, 2020
@arendst
Copy link
Owner

arendst commented Jun 12, 2020

Thx.

arendst added a commit that referenced this pull request Jun 12, 2020
Add support for HP303B Temperature and Pressure sensor by Robert Jaakke (#8638)
@BytesOnline
Copy link

How can this device be used?

The scan finds a device:
stat/tasmota_24B1D5/RESULT = {"I2CScan":"Device(s) found at 0x77"}

But which driver is this or how can I enable it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold by dev team Result - Feature request put on hold by member of development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants