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

ms5611 fix #969

Closed
wants to merge 1 commit into from
Closed

Conversation

truglodite
Copy link

@truglodite truglodite commented Jan 31, 2020

Description:

ESPhome ms5611 library results in errors in pressure readings (jump up/down by 30hpa randomly). This is likely due to a problem with reading the prom calibration values. I have used another library successfully on other projects for several years straight. This ms5xxx library is found here:
https://github.com/Schm1tz1/arduino-ms5xxx

The only related difference between the libraries that I found so far is a different delay bewteen the reset command and prom read command. ESPhome uses 100ms, ms5xxx uses 3ms. This very well could be the cause of the problem!

ms5xxx also variable delays between readings and conversions that depend on resolution. Esphome uses 10ms for these delays, which is the same as the ms5xxx delay for 4k resolution. Since I'm not sure which res esphome uses, I left those at 10ms for now.

A link to the original discussion with some troubleshooting, and the issue report:
https://community.home-assistant.io/t/esphome-ms5611-issue/164240/2
esphome/issues#1004

[update: The erroneous measurements still occer with this change.]

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:

@truglodite
Copy link
Author

I scanned over the available generic arduino "ms5611" libraries in PIO, and I noticed all of them use delay(3) or at most delay(4) in that spot. The esphome library is obviously based on Jarzebski's arduino ms5611 library, which is the only one I've seen with that eternal delay(100).

@truglodite
Copy link
Author

It was a struggle, but I managed to get firmware compiled with the changes included. Testing as of now... will report back as soon as I see any issues (hopefully none).

@truglodite
Copy link
Author

Unfortunately, the 10ms fix alone did not help. I'm not sure what else it could be; maybe it is something else in the library, or something in the way esphome handles the library? IDK, but a fix is over my head at this point.

@truglodite
Copy link
Author

truglodite commented Feb 4, 2020

I turned my attention to the calculation section and found many differences in math, but I believe most of it is equivalent (bitshifts vs pow(2,x), etc). However I'm not familiar with the methods used in esphome. So I don't know for sure if it has issues with handling overflows, etc. The ms5xxx maths are known to work well. So I tried implementing them as 'literally' as possible, without success (values were silly higher than they should be).

Here are the "good maths" copied directly from ms5xxx.cpp (read_adc() returns an unsigned int, and C[] is an array of unsigned int's... also note C[0] isn't used so indexes are shifted up 1):

	unsigned long D1=0, D2=0;
	
	double dT;
	double OFF;
	double SENS;

	D2=read_adc(MS5xxx_CMD_ADC_D2+MS5xxx_CMD_ADC_4096);
	D1=read_adc(MS5xxx_CMD_ADC_D1+MS5xxx_CMD_ADC_4096);

	// calculate 1st order pressure and temperature (MS5607 1st order algorithm)
	dT=D2-C[5]*pow(2,8);
	OFF=C[2]*pow(2,17)+dT*C[4]/pow(2,6);
	SENS=C[1]*pow(2,16)+dT*C[3]/pow(2,7);
	TEMP=(2000+(dT*C[6])/pow(2,23));
	P=(((D1*SENS)/pow(2,21)-OFF)/pow(2,15));
	 
	// perform higher order corrections
	double T2=0., OFF2=0., SENS2=0.;
	if(TEMP<2000) {
	  T2=dT*dT/pow(2,31);
	  OFF2=61*(TEMP-2000)*(TEMP-2000)/pow(2,4);
	  SENS2=2*(TEMP-2000)*(TEMP-2000);
	  if(TEMP<-1500) {
	    OFF2+=15*(TEMP+1500)*(TEMP+1500);
	    SENS2+=8*(TEMP+1500)*(TEMP+1500);
	  }
	}
	  
	TEMP-=T2;
	OFF-=OFF2;
	SENS-=SENS2;
	P=(((D1*SENS)/pow(2,21)-OFF)/pow(2,15));

...and my failed attempt at implementing the same math functions in esphome (values are way too high to be right):

void MS5611Component::calculate_values_(uint32_t d2, uint32_t d1) {
	// Truglodite: adapted calcs from ms5xxx.cpp
	double dT;
	double OFF;
	double SENS;
	unsigned long D1=d1;
	unsigned long D2=d2;

	dT=D2 - double(this->prom_[4]) * pow(2.0,8.0);
	OFF=double(this->prom_[1])*pow(2.0,17.0)+dT*double(this->prom_[3])/pow(2.0,6.0);
	SENS=double(this->prom_[0])*pow(2.0,16.0)+dT*double(this->prom_[2])/pow(2.0,7.0);
	double TEMP=(2000.0+(dT*double(this->prom_[5]))/pow(2.0,23.0));
	double P=(((D1*SENS)/pow(2.0,21.0)-OFF)/pow(2.0,15.0));

	double T2=0., OFF2=0., SENS2=0.;
	if(TEMP<2000.0) {
	  T2=dT*dT/pow(2.0,31.0);
	  OFF2=61.0*(TEMP-2000.0)*(TEMP-2000.0)/pow(2.0,4.0);
	  SENS2=2.0*(TEMP-2000.0)*(TEMP-2000.0);
	  if(TEMP<-1500.0) {
		OFF2+=15.0*(TEMP+1500.0)*(TEMP+1500.0);
		SENS2+=8.0*(TEMP+1500.0)*(TEMP+1500.0);
	  }
	}

	TEMP-=T2;
	OFF-=OFF2;
	SENS-=SENS2;
	P=(((D1*SENS)/pow(2.0,21.0)-OFF)/pow(2.0,15.0));

	ESP_LOGD(TAG, "Got temperature=%0.02f°C pressure=%0.01fhPa", float(TEMP), float(P));

	if (this->temperature_sensor_ != nullptr)
	this->temperature_sensor_->publish_state(float(TEMP));
	if (this->pressure_sensor_ != nullptr)
	this->pressure_sensor_->publish_state(float(P));  // hPa
	this->status_clear_warning();
}

I thought maybe there was a typo in the esphome library on those prom read indexes, so I tried shifting them up 1 to look like ms5xxx. That failed with even worse values, but worth noting it was
at least tested. Other than that, maybe someone with more skill can see this, glance at the esphome ms5611.cpp, and know exactly what's wrong?

@stale
Copy link

stale bot commented Apr 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 9, 2020
@stale stale bot closed this Apr 16, 2020
@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
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

2 participants