search

RobTillaart / MS5611

Arduino library for MS5611 temperature and pressure sensor
MIT License
17 stars 5 forks source link

Add SPI Support #17

Closed LyricPants66133 closed 2 years ago

LyricPants66133 commented 2 years ago

(NOT READY FOR MERGE)

Added SPI support with minimal work required to use SPI. It uses all of the exact same function calls, with the exception of a minor change in the constructor for SPI.

Using preprocessors, the compiler only needs to import the required library for a communications protocol. This ensures that the other protocol is not redundantly included. This however does make the code a bit messier, but that can always be improved. To change protocols, the user must go into the header file and change a definition. I2C is the default protocol, so this should affect a minimal amount of users that need SPI.

As mentioned earlier, this code is not ready for merge. There is an odd issue with the SPI interface where the temperature readings are off by about +10C and increase rapidly. Pressure readings remain accurate. This issue should be fixed before a merge. I would also like to improve and clean up the code a bit more as well, but that is not currently essential.

RobTillaart commented 2 years ago

You are doing a great job, please notice there is an issue #16 for which a fix will be merged soon. so far it is only a change of variable names (conflict on MBED platform, NANO BLE)

I will look into your proposals later today / tomorrow.

RobTillaart commented 2 years ago

Had a quick look and I propose to make the current I2C based class a base class from which a MS5611_SPI class is derived.

That does not make your effort useless, on contrary, I see it as an important step investigating the working of the SPI version.

RobTillaart commented 2 years ago

Three classes would be best. Crossed my mind too. Reason for i2c as base class was backwards compatibility for existing code but that is still possible.

Base_ms5611. <<<<. Ms5611 Base_ms5611 <<<<. Ms5611_spi Base_ms5611. <<<<. Optimized versions ...?

RobTillaart commented 2 years ago

@LyricPants66133 Seems that your posts get lost. Are you deleting them? It makes this discussion thread useless and these old threads often catch the rationale why changes happen.

LyricPants66133 commented 2 years ago

I don't remember deleting that message. That's odd. For future reference, I suggested having three different classes. One class each for I2C and SPI communication, and a third shared class with common functions that are not protocol specific. This would make both SPI and I2C equivalent communication methods, and would also make it easier for a user to exclude one for reduced memory consumption.

As a current note, I should be making a commit later today.

RobTillaart commented 2 years ago

OK, thanks for the clarification.

LyricPants66133 commented 2 years ago

Could maybe use some cleaning up, but this is a lot cleaner than it was before. Temperature readings with SPI still do not work.

If we really want to make it easier for users to fully exclude either of the protocols, we could use a couple of #ifdefs( a lot less than before). However, as you mentioned earlier, they could conflict with other libraries.

RobTillaart commented 2 years ago

looked into the files in - LyricPants66133:master - and 100% confirmed this is the better structure.

Temperature readings with SPI still do not work.

Do you get raw data?

With SPI devices I do the select device before the beginTranaction (see below). Could be a timing difference.

int MS5611_SPI::command(const uint8_t command)
{
  yield();

  digitalWrite(_address, LOW);          // select device
  _SPI->beginTransaction(_spiSettings); // start SPI
  _SPI->transfer(command);              // send command
  _result = _SPI->transfer(0x00);       // receive first byte
  _SPI->endTransaction();               // end SPI
  digitalWrite(_address, HIGH);         // de-select device

  return _result;
}
LyricPants66133 commented 2 years ago

Hey Rob, thanks for your feedback. I tried your changes, but they did not work. I definitely get raw values within an expected range. Upon closer inspection, it became apparent that the issue is a bit worse than I initially thought. Pressure readings are also off, but it was not apparent to me before. Pressure readings remain rather constant at -10 what I2C reads, while temperature readings trend upwards over time. PROM reads correctly, so its definitely something to do with read, convert, and readadc methods in the SPI class.

PS Happy New Year!

RobTillaart commented 2 years ago

(quite busy today, so very little time)

As the constants are read correctly, it is strange.

what do you use for spiSettings ?
which mode? Is there a hardware line that needs to be kept LOW/HIGH when using SPI?

RobTillaart commented 2 years ago

@LyricPants66133 Had a "fight" with this library in combination with the NANO 33 BLE which resulted in a new develop branch.

This will probably be merged soon, tomorrow additional tests will be done. Should not give too much problems merging,

just to let you know.

BTW - any progress made?

LyricPants66133 commented 2 years ago

@RobTillaart Hello Rob! Apologies for the hiatus. I've been a bit busy the past week. Unfortunately I have not made too much progress. I tested many different SPI settings, however discounted that as the causation of the problem due to the fact that PROM reading works. I think I may have isolated the issue down to a specific sentence within the documentation:

"If the ADC read command is sent during conversion the result will be 0, the conversion will not stop and the final result will be wrong. Conversion sequence sent during the already started conversion process will yield incorrect result as well."(page 11).

The ADC command 0x00 is also the command to read output. This means that all instances of _SPI->transmit(0x00); that are not reading ADC will have to be removed. Unfortunately this will lead to the library operating in an 'optimistic' manner. However I will do more testing this weekend as I am free again.

It is also entirely possible that there is a hardware issue with my specific chip, however this is unlikely.

I will also look at merging the other branch into this draft PR once it gets merged to master. Thanks Rob!

RobTillaart commented 2 years ago

There are more (minor) changes in the pipeline. It might be a better idea to just get a standalone SPI version first. (just thinking out loud)

RobTillaart commented 2 years ago

@LyricPants66133

Created a SPI library for the MS5611 today based on the I2C one, so quite some reuse of higher level code.

UNO

NANO 33

ESP32

snippet from ESP32 output HW SPI - (VSPI pins) Hex numbers are the raw _D1 and _D2.

21:58:02.404 -> 8E7DE0
21:58:02.404 -> 7EFFE0
21:58:02.404 -> T:  23.91   P:  1033.50       (temp should be around 19-20;   pressure = 1035 hPa so within 0.2%)
21:58:04.412 -> 8E7BE0
21:58:04.412 -> 7F01E0
21:58:04.412 -> T:  23.92   P:  1033.44

The ESP32 has other code paths than the UNO/NANO so that might explain why HW_SPI is working for EPS32.

Big problem is that all test showed a problem with temperature, which I do not see with the I2C connection. This can be caused by internal heating of the IC when SPI logic is used. At 5V SPI connections (UNO) this was more explicit than on the 3V3 ESP32. More investigations are needed as this is not reliable enough to share. Hope to get a final verdict tomorrow.

LyricPants66133 commented 2 years ago

Thanks, Rob! I've been a bit busy, however, I was able to conduct some testing. I'm using an Arduino Nano and a GY-63. As mentioned earlier, I still exhibit issues on both readings. It is slightly reassuring to see that you also experience these issues. In my testing, I tried out some other SPI libraries, however, I never get values that match/are similar to Wire. I am unsure if anything can be done to mitigate these issues.

As you pointed out, they may be caused by hardware heating. This may explain why PROM values are correct, but not pressure/temperature values. I did also notice that the rapidly increasing pressure/temperature values eventually leveled out when I left my sensor running long enough. This may be the hardware reaching its peak temperature during operation.

I appreciate all of the work you are putting in while I am busy.

RobTillaart commented 2 years ago

What is also important to keep in mind is that given the explicit needed delays, the SPI version won't be substantial faster. Did no detailed performance investigation yet.

Besides the question at the other lib builder no real progress has been made. If the heating is the cause there are 2 candidates. Chip select line and the protocol select line. These are not used with I2C.

RobTillaart commented 2 years ago

looking at the schema of the GY-63 I see no indication for extra heat production except for R2 when PS = GND = SPI. but that is an external resistor and it is only ~11 mW (3V3 over 1K = 3.3 mA ==> 11 mW)

Clock and Data IN are both reduced to 3V3 so within specs and DATA OUT is OUTPUT so it will not exceed 3V3 (I assume)

https://www.smart-prototyping.com/image/data/2020/10/102074%20MS5611/GY-63-SCH.jpg

Brings no new clues for now.

RobTillaart commented 2 years ago

@LyricPants66133

Created a repository for my SPI version - https://github.com/RobTillaart/MS5611_SPI

Not released or registered yet as the internal heating problems are still there.

LyricPants66133 commented 2 years ago

@RobTillaart Thanks for creating that experimental repo and improving the code. I only have a Nano and Uno so your additional testing is great. I did some testing on my GY-63, and the temperature issues are 100% caused by the sensor heating itself by the continuous feeding of power. This issue cannot be mitigated by adding delays between readings. It may be worth investigating if other MS5611 equipped sensors such as the GY-86 also encounter this issue.

Also, I wont resolve conflicts until we near a merge simply to do it in one go.

RobTillaart commented 2 years ago

With I2C the sensor is also powered constantly. So is it the Protocol Select line? Mmm. Could make that line switchable, set it to i2c when "disabled"... Need to check hardware design again, and imagine the heating scenario per difference with i2c.

(Pressed close by accident)

Do you know where to buy GY86? Tbc tomorrow.

RobTillaart commented 2 years ago

Is the GY86 with all those other sensors I2C only? Do you know?

RobTillaart commented 2 years ago

Is the GY86 with all those other sensors I2C only? Do you know?

YES, although physical layer connector looks similar, it is I2C only.

See - https://www.smart-prototyping.com/image/data/2_components/Arduino/100960%20MPU6050/01.jpg

RobTillaart commented 2 years ago

Some small tests this morning:

1) connect the PS line to GND using a resistor of ~ !K to reduce current ==> still heating 2) connect the PS line to CSO line so it will only go low when SPI mode is needed ==> still heating. 3) current through PS line 0.5-0.7 uA (with 1K ) so not expected as heat source. 4) current through CS line 1.5-2.2 uA (with 1K ) so not expected as heat source.

RobTillaart commented 2 years ago

ESP32 running for 2+ hrs - seems to stabilize around 23C while room is 19.5C I'm now going to drop the read frequency to see how that affects Temp.

RobTillaart commented 2 years ago

ESP32 running for 2+ hrs - seems to stabilize around 23C while room is 19.5C I'm now going to drop the read frequency to see how that affects Temp.

1:30 at once per 10 seconds give 22.9C so roughly same..

RobTillaart commented 2 years ago

Next test: Setting MOSI to HIGH between reads instead of LOW -- using software SPI.

RobTillaart commented 2 years ago

Next test: Setting MOSI to HIGH between reads instead of LOW -- using software SPI.

drops ==> longer test started...

LyricPants66133 commented 2 years ago

Apologies for not replying earlier on the GY-86. You however seem to have answered that it is only I2C. Your tests will definitely be great in pinpointing the source of heat. I find it interesting that ESP32 SPI runs so cool. My Nano/GY-63 runs at 37°C with room temperature at 21°C.

Something to note however is the fact that the temperature ramps up over time. If the sensor is left unplugged, it will obviously remain at room temp. However if it is plugged in with no code running, will the temperature change?

After running the Arduino with no code, then loading a test script running on OSR_ULTRA_LOW and immediately opening the serial monitor, my first measurement value for temperature was 25.39°C. It rapidly increased. This might mean that the fact of simply beginning an SPI connection raises its temps. More investigation is obviously needed.

RobTillaart commented 2 years ago

I will release the spi version of my lib soon, with a warning in the readme.md file. Protocol wise it works as it should and the incorrect readings are not caused by the library as far as we know.

Long run with mosi did not solve it. Just slightly better, not enough to conclude it is significant.

Changing the SPI clock frequency did not solve it either..

There are a few tests left that I will do asap. I expect the solution will be a big 'Why didn't i think if that before'. Googled yesterday quite a bit but there are few SPI libraries out there and only one other reference of self heating found on electronic stack exchange. No solution. 99.99% is I2C

To be continued

LyricPants66133 commented 2 years ago

Yes that would probably be our best step forward. If you want any help merging and resolving conflicts, or adding any other features, please tell me.

RobTillaart commented 2 years ago

Released 0.1.0 of the MS5611_SPI library and registered it in the Arduino library manager / platformIO. No new (practical) ideas how to prevent internal heating (OK shutdown VCC altogether).

So if you have ideas how to explain and solve the internal heating let me know.

The libraries will not be merged. At least the heating problem should be understood and solved. The SPI lib will follow the I2C lib in terms of functional interface.

LyricPants66133 commented 2 years ago

Cool! If I come up with any ideas, I'll make sure to let you know.

Also, can I ask why the libraries won't be merged? I feel like that would make it harder to have common features up to date on both libraries.

And would you like me to close the pr?

RobTillaart commented 2 years ago

Also, can I ask why the libraries won't be merged?

Yes you may.

  1. The footprint would become too large for the smaller processors as it would include both protocol stacks while (most often) only one is used
  2. The SPI version has an not understood problem (heating). That must be solved before merge as it would downgrade the library otherwise. Now I have one "100% OK" I2C version and a sloppy SPI version. After merge only the sloppy version would survive,
  3. Keeping them in sync is not so difficult. Work in small steps and sync a.s.a.p.

And would you like me to close the pr?

please. Thanks for the effort you put into this so far! is appreciated!

RobTillaart commented 2 years ago

PS, by any change do you know which device are compatible with MS5611 ? sometimes I see MS5607 and MS57?? series Could be worth to upgrade the readme with this info (in some next release)

RobTillaart commented 2 years ago

FYI - created an issue with notes about the heating problem - https://github.com/RobTillaart/MS5611_SPI/issues/3

RobTillaart commented 2 years ago

PS, by any change do you know which device are compatible with MS5611 ? sometimes I see MS5607 and MS57?? series Could be worth to upgrade the readme with this info (in some next release)

You might have a look at the MS8607 - no experience myself but I heard this in another discussion.

LyricPants66133 commented 2 years ago

Hey Rob, Apologies for the late reply. From what I can see, some other MS type devices use a similar communication protocol, for example the MS5607-02BA03 does seem to use the same protocol. Looking at the TE Connectivity website, this should be a filtered out list of 'compatible' devices.(Compatible in quotes because Im making assumptions.)

Your research into the heating problem looks great. Thank you so much.

And the MS8607 also looks like a promising alternative to the MS5611, thanks for bringing it to my attention.