Changes to allow code to run on SparkFun OpenLog Artemis #22

PaulZC · 2021-01-27T07:59:07Z

We really like the BAR02 sensor and want to add support for it on OpenLog Artemis.
To do that, we need to remove the hardwiring to the Wire interface. This PR does that.

Please note: I haven't updated the version in library.properties. You might want to do that after merging.

Changes:

Moved the .h and .cpp files into a /src folder
Change init to init(TwoWire &wirePort = Wire);. wirePort is optional and will default to Wire so all existing code will continue to work normally
Add .begin which simply calls init
Add the first part of @NeilBaner 's PR Fix compiler errors when compiling for Arduino Nano 33 BLE platform #21 to allow the code to compile on Mbed platforms
Add a slightly different fix for the second part of Fix compiler errors when compiling for Arduino Nano 33 BLE platform #21 - changed the #defines to const uint8_t
Add keywords.txt
Add a second example - for the BAR02

I haven't included @senceryazici 's PR #16 to support the other OverSample Rates but his changes look good. You might want to merge those too.

Enjoy!

Paul

…PR bluerobotics#21

…e port to use. Adding .begin Add wirePort as a parameter for .begin and .init Add _i2cPort (Private) Change Wire. to _i2cPort->

patrickelectric

Hi @PaulZC!
Thanks for the great contribution, it indeed is a great thing that you are doing!
Just some small corrections and a couple of suggestions to get it merged :)

patrickelectric · 2021-02-03T15:59:14Z

src/MS5837.cpp

@@ -19,110 +19,140 @@ MS5837::MS5837() {
 	fluidDensity = 1029;
 }

-bool MS5837::init() {
+bool MS5837::begin(TwoWire &wirePort) {
+	return (init(wirePort));


You could add wirePort.begin() here also.
Also, is there a reason for you do choose this approach and not to add TwoWire as a class constructor argument ?

Hi Patrick (@patrickelectric),
Thanks for the feedback!
We don't call Wire.begin inside the library. We expect the user to call Wire.begin externally. It saves problems with (e.g.) (re)setting the clock speed. Please see this post (scroll down to the Don’t call Wire.begin() in the library section).
https://www.sparkfun.com/news/3245
Yes, true, you could use a class constructor argument. We choose not to do it that way.
Paul

patrickelectric · 2021-02-03T16:09:49Z

src/MS5837.cpp

+}
+
+bool MS5837::init(TwoWire &wirePort) {
+	_i2cPort = &wirePort; //Grab which port the user wants us to use


You could move _i2cPort from ptr to reference if you do the initialization of it in the constructor

Yes, true, but we choose not to do it that way. All the SparkFun libraries accept the wirePort as an parameter in .begin. There are some devices that can communicate over both I2C or SPI and we deal with those by overloading .begin.

patrickelectric · 2021-02-03T16:10:33Z

src/MS5837.cpp

+		_model = MS5837_02BA;
+		return true;
+	}
+	else if (version == 0x15) // MS5837-02BA21


else unnecessary after return

Ah, yes, you're right. I've changed this and have added more improvements in the latest commits:

I've defined a const uint8_t for each sensor version (based on the values in the datasheets)

I've added MS5837_UNRECOGNISED as a possible value for _model

.init sets _model according to the sensor version it extracts from PROM Word 0

if the sensor version is unrecognised, .init still returns true but sets _model to MS5837_UNRECOGNISED

the code is still backward-compatible. You can still call setModel to override _model if required. (E.g. if TE has used a different sensor version ID which they have forgotten to tell us about!)

The main reason I've added all of this is that we support the MS5637 on OpenLog Artemis. It has the same address as the MS5837 and has the same memory structure. It passes the same CRC test. Before I made these changes, the MS5837 was appearing as an MS5637. The MS5637 datasheet doesn't define what the 'sensor version' bits are, so I can't update the MS5637 library with the same changes... These changes mean I can test for an MS5837 first. If _model is MS5837_UNRECOGNISED then I know I'm almost certainly talking to a MS5637.

Anyway, I think we've ended up with a nice solution. Everything is backward-compatible. .init sets the _model automatically, but you can override it if you want to.

Cheers!

Paul

patrickelectric · 2021-02-03T16:10:42Z

src/MS5837.cpp

+		_model = MS5837_02BA;
+		return true;
+	}
+	else if (version == 0x1A) // MS5837-30BA26


You could also break it to a else case

Please see the comment above...

patrickelectric · 2021-02-03T16:10:55Z

src/MS5837.cpp

-	Wire.beginTransmission(MS5837_ADDR);
-	Wire.write(MS5837_CONVERT_D1_8192);
-	Wire.endTransmission();
+	_i2cPort->beginTransmission(MS5837_ADDR);


Check if i2cPort is not nullptr

Sure. OK. Resolved here:
2b5747d#diff-e98a2dd452878f22bfa9405f77a816ec8c6add4277c3d61c18ef3ed2a98ad598R91-R96

patrickelectric · 2021-02-03T16:11:27Z

src/MS5837.h

@@ -50,14 +51,16 @@ class MS5837 {

 	MS5837();

-	bool init();
+	bool init(TwoWire &wirePort = Wire);
+  bool begin(TwoWire &wirePort = Wire); // Calls init()


wrong indentation

Pesky whitespace! Resolved here:
2b5747d#diff-fedaeb63fea75cce65adad3c7c284f5f797befa455548788021315c68d0f423cR55

patrickelectric · 2021-02-03T16:11:51Z

src/MS5837.h


 	/** Set model of MS5837 sensor. Valid options are MS5837::MS5837_30BA (default)
 	 * and MS5837::MS5837_02BA.
 	 */
 	void setModel(uint8_t model);
+	uint8_t getModel(void);


void in constructor is unnecessary in C++ code

OK. Resolved here:
2b5747d#diff-fedaeb63fea75cce65adad3c7c284f5f797befa455548788021315c68d0f423cR61
and here:
2b5747d#diff-e98a2dd452878f22bfa9405f77a816ec8c6add4277c3d61c18ef3ed2a98ad598R82

…ISED. .init returns true even if the sesnor version is unrecognised.

…el according to the sensor version.

…e sensor version if required

patrickelectric · 2021-02-08T13:32:39Z

Hi @PaulZC, it appears that your PR is failing, can you rebase it over master branch ?

PaulZC · 2021-02-08T14:19:50Z

Done!
Best wishes,
Paul

PaulZC · 2021-02-17T21:05:12Z

Hi @patrickelectric ,
Are you able to merge this PR now?
Best wishes,
Paul

jaxxzer · 2021-03-02T20:25:23Z

Bump @patrickelectric

patrickelectric · 2021-03-03T13:18:34Z

Hi @PaulZC!
I did the final tests and everything appears to be working, to avoid more work from your part, I did a couple of corrections and organization on the commit history, they are already on our master branch.
Thanks for you contribution and sorry for the delay!

patrickelectric

Merged manually.

PaulZC · 2021-03-03T13:33:07Z

No problem @patrickelectric (& @jaxxzer) - thank you!
All the best,
Paul

PaulZC added 10 commits January 27, 2021 06:05

Create MS5837_Example_BAR02.ino

6a7798d

Moving .h and .cpp into src

7041290

Create keywords.txt

f11e5f3

Adding the first part of @NeilBaner PR bluerobotics#21

d202ac2

Change #defines to const uint8_t - this replaces part 2 of @NeilBaner …

6da6a64

…PR bluerobotics#21

Adding .begin

d6e63a1

Remove the hardwiring to Wire. Allow the user to specify which TwoWir…

cd0b528

…e port to use. Adding .begin Add wirePort as a parameter for .begin and .init Add _i2cPort (Private) Change Wire. to _i2cPort->

Update README.md

568d20e

.begin now checks the sensor version and sets _model accordingly

4d650d9

Adding getModel

78f7f92

patrickelectric requested changes Feb 3, 2021

View reviewed changes

PaulZC added 4 commits February 4, 2021 07:33

Changes based on feedback from @patrickelectric

2b5747d

Adding const uint8_t's for the sensor version. Adding MS5837_UNRECOGN…

2f496e9

…ISED. .init returns true even if the sesnor version is unrecognised.

Deleting the BAR02 example - it is redundant now that .init sets _mod…

d82d23d

…el according to the sensor version.

Updating the example to show that setModel can be used to override th…

1bf4e1c

…e sensor version if required

Merge remote-tracking branch 'upstream/master'

2399b3b

patrickelectric approved these changes Mar 3, 2021

View reviewed changes

patrickelectric closed this Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to allow code to run on SparkFun OpenLog Artemis #22

Changes to allow code to run on SparkFun OpenLog Artemis #22

PaulZC commented Jan 27, 2021

patrickelectric left a comment

patrickelectric Feb 3, 2021

PaulZC Feb 4, 2021

patrickelectric Feb 3, 2021

PaulZC Feb 4, 2021

patrickelectric Feb 3, 2021

PaulZC Feb 5, 2021 •

edited

Loading

patrickelectric Feb 3, 2021

PaulZC Feb 5, 2021

patrickelectric Feb 3, 2021

PaulZC Feb 4, 2021

patrickelectric Feb 3, 2021

PaulZC Feb 4, 2021

patrickelectric Feb 3, 2021

PaulZC Feb 4, 2021

patrickelectric commented Feb 8, 2021

PaulZC commented Feb 8, 2021

PaulZC commented Feb 17, 2021

jaxxzer commented Mar 2, 2021

patrickelectric commented Mar 3, 2021

patrickelectric left a comment

PaulZC commented Mar 3, 2021

Changes to allow code to run on SparkFun OpenLog Artemis #22

Changes to allow code to run on SparkFun OpenLog Artemis #22

Conversation

PaulZC commented Jan 27, 2021

patrickelectric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulZC Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickelectric commented Feb 8, 2021

PaulZC commented Feb 8, 2021

PaulZC commented Feb 17, 2021

jaxxzer commented Mar 2, 2021

patrickelectric commented Mar 3, 2021

patrickelectric left a comment

Choose a reason for hiding this comment

PaulZC commented Mar 3, 2021

PaulZC Feb 5, 2021 •

edited

Loading