adafruit/Adafruit_AMG88xx

Bit masking and shifting when setting interrupt thresholds are incorrect.

HelloHungryImDad opened this issue · 9 comments

HelloHungryImDad commented

Fixed in #29.

The code for setting the upper and lower thresholds, and the hysteresis, should split the 16 bit converted values into a lower byte and an upper nibble (12 bit value total) as per the register diagram posted below.

The code (highConv & 0xF) >> 4; currently takes the top 4 bits of the lower byte (0xF) and shifts them right 4 bits. The upper byte is completely discarded. To actually get the lower 4 bits of the upper byte the bit mask should be 0xF00 and then shifted right 8 bits.

This is the same for the high threshold, low threshold, and hysteresis registers.

caternuson commented

Just adding datasheet info for these registers for reference:
image

HelloHungryImDad commented

Thanks @caternuson. So currently the library sets INTHH = INT_LVL_H [7:4], INTLH = INT_LVL_L [7:4], and INT_HYSH = INT_HYS[7:4] instead of [11:8] as requested by the datasheet.

caternuson commented

Yep, agree. Were you able to test this? Seems fine just looking at code, but would be good to also know hands on.

HelloHungryImDad commented

That's how I found the bug. Empirically. I patched my local code, verified it works and raised the PR here.

Try setting the any of the thresholds to a 16 bit number higher than 255 and read the registers back. You'll see what I mean.

caternuson commented

Cool, thanks. Trust your testing. Just wanted to make sure this was more than just code inspection.

HelloHungryImDad commented

Thanks for providing the library! If I do much more with this hardware, I'll be sure to contribute any other features I add.

caternuson commented

Please try the 1.3.1 release when it becomes available via Library Manager:
https://github.com/adafruit/Adafruit_AMG88xx/releases/tag/1.3.1

HelloHungryImDad commented

Just tried it. All good!

caternuson commented

Thanks!