BUG and Improvement suggestions

[williamesp2015]

Very good program but I should say 1) move initializing I2C before reset the MAX30102 because you haven't start I2C but you communicated with it. 2) Set clock to 400 k 3) make it custom pin config like this: Wire.begin(SDA_Pin,SCL_Pin); Wire.setClock(400000L); maxim_max30102_reset(); //resets the MAX30102

Suggestions: MAX30101 is superior to the MAX30102 in term of accuracy and lower noise, so I would be grateful if you consider minor changes like these into you code: max30102.h:

define REG_LED3_PA 0x0E // I ADDED

//Multi-LED Mode configuration (pg 22) static const uint8_t MAX30102_SLOT1_MASK = 0xF8; static const uint8_t MAX30102_SLOT2_MASK = 0x8F; static const uint8_t MAX30102_SLOT3_MASK = 0xF8; static const uint8_t MAX30102_SLOT4_MASK = 0x8F;

static const uint8_t SLOT_NONE = 0x00; static const uint8_t SLOT_RED_LED = 0x01; static const uint8_t SLOT_IR_LED = 0x02; static const uint8_t SLOT_GREEN_LED = 0x03; static const uint8_t SLOT_NONE_PILOT = 0x04; static const uint8_t SLOT_RED_PILOT = 0x05; static const uint8_t SLOT_IR_PILOT = 0x06; static const uint8_t SLOT_GREEN_PILOT = 0x07; uint8_t getRevisionID(); void setPulseAmplitudeGreen(uint8_t amplitude); void bitMask(uint8_t reg, uint8_t mask, uint8_t thing);

max30102.cpp: in bool maxim_max30102_init(): // I ADDED if(!maxim_max30102_write_reg(REG_LED3_PA,0x24)) // Choose value for ~ 7mA for LED3 = GREEN in MAX30101 return false; bitMask(REG_MULTI_LED_CTRL2, 0xF8,0x03);//MAX30105_SLOT3_MASK=0xF8 ///// // I Added // // Device ID and Revision // uint8_t readPartID() { uint8_t ChipID; maxim_max30102_read_reg(REG_PART_ID , &ChipID); return ChipID; }

uint8_t getRevisionID() { uint8_t ChipRev; maxim_max30102_read_reg(REG_REV_ID , &ChipRev); return ChipRev; } void setPulseAmplitudeGreen(uint8_t amplitude) { maxim_max30102_write_reg(REG_LED3_PA, amplitude); } //Given a register, read it, mask it, and then set the thing void bitMask(uint8_t reg, uint8_t mask, uint8_t thing) { uint8_t originalContents =0; // Grab current register context maxim_max30102_read_reg(reg,&originalContents); // Zero-out the portions of the register we're interested in originalContents = originalContents & mask; // Change contents maxim_max30102_write_reg(reg, originalContents | thing); } //Report the most recent Green value uint32_t getGreen(void) { do somthing } Add related changes in the RD117_Arduino.ino

Many thanks

[aromring]

Hi William, Thank you for the bug report, but I am quite puzzled. In my RD117_ARDUINO.ino the I2C initialization:

line 85: Wire.begin();

is (and has been) executed before the MAX30102 reset

line 92: maxim_max30102_reset(); //resets the MAX30102

Hence, I am not sure what you are referring to. There is no bug, is there? Were you actually looking at RD117_ARDUINO.ino in some of the cloned repositories? If yes, then you should contact respective authors.

[williamesp2015]

Hi aromring,

Yes, you did in line 85 but again wire.begin() started in the maxim_max30102_init(); so if anyone has custom I2C pin, it will rest the actual config. So my suggestion to have custom pins and high speed I2C is: Wire.begin(SDA_Pin,SCL_Pin); Wire.setClock(400000L); maxim_max30102_reset(); //resets the MAX30102 Regarding my suggestion for the MAX30101, I tested your code with MAX30101 but output for REF results were 0. Could you consider adding MAX30101?

[aromring]

OK, I see what you mean: It's not that "I2C is not initialized", it's about Wire.begin() being called twice! Yes, it's a bit unnecessary and actually buggy when someone uses custom I2C pins. Here is what I am planning to do to fix this:

1) Remove all Wire references from RD117_ARDUINO.ino entirely. The only place with I2C calls should be in max30102.(h/cpp). 2) Move maxim_max30102_reset() and maxim_max30102_read_reg(REG_INTR_STATUS_1,...) into the maxim_max30102_init() - this is the right place for these calls. It won't happen today, I am afraid. These changes must be tested before checking in. If you noticed, it took me 2 (!) days to find time for this answer. Which means I am very busy with my job and family. Perhaps this weekend, but I can offer no guarantees.

Now, to address your suggestions (which are good, by the way): I can't implement them in this repository for two reasons: 1) I simply don't have time, as I wrote above. I did this project years ago and currently can offer only basic support and bug fixes. 2) I want to keep this code as agnostic as possible. I was already getting grief for sticking ADALOGGER/SD code in it, which forced me to put a whole bunch of #ifdef 's all over the code. Hence, adding new platforms is the last thing on my mind.

There is much better way to implement your suggestions: fork this repository into your Git account, modify the code accordingly, the announce to the world: "Hey, look, here is a better version of this code that does X, Y, and Z". I have no problem with that at all! In fact, there are already 55 forks of MAX30102_by_RF. Maybe some of these are tailored towards MAX30101?

[aromring]

The promised changes (plus die temperature readings requested a year ago) have been implemented in commit f6160ab today (10/10/2021).