physical-computation / Warp-firmware

Firmware for the Cambridge Physical Computation Laboratory's Warp Embedded Multi-Sensor Platform.
http://physcomp.eng.cam.ac.uk
BSD 3-Clause "New" or "Revised" License
5 stars 198 forks source link

Incorrect bme680 calibration register address #93

Open Tom-Newton opened 3 years ago

Tom-Newton commented 3 years ago

This doesn't actually seem to cause any problems though I suspect if compiled in debug mode this would cause crashes due to trying to write to unallocated memory. I just spotted this when looking at the code.

The number of bme680 calibration constants: https://github.com/physical-computation/Warp-firmware/blob/96e67b97361f817768550fd1368a80a67fc46264/src/boot/ksdk1.1.0/warp.h#L170 is not consistent with the range of addresses it attempts to read from: https://github.com/physical-computation/Warp-firmware/blob/96e67b97361f817768550fd1368a80a67fc46264/src/boot/ksdk1.1.0/warp.h#L222-L225

These constants are used here: https://github.com/physical-computation/Warp-firmware/blob/96e67b97361f817768550fd1368a80a67fc46264/src/boot/ksdk1.1.0/devBME680.c#L172-L186 and this declaration is relavent https://github.com/physical-computation/Warp-firmware/blob/96e67b97361f817768550fd1368a80a67fc46264/src/boot/ksdk1.1.0/warp-kl03-ksdk1.1-boot.c#L145 These for loops read from 42 different memory locations and write the contents to an array of length 41.

I believe the fix is to set this: https://github.com/physical-computation/Warp-firmware/blob/96e67b97361f817768550fd1368a80a67fc46264/src/boot/ksdk1.1.0/warp.h#L225 to 0xf1.

Hopefully I haven't just completely misunderstood something here.

JamesTimothyMeech commented 3 years ago

Hello Tom, sorry about the delay in getting back to you on this.

Looks like you're correct and there is a mistake here

Would you like to go ahead and make a pull request for this so you get credit for the fix?

Or would you prefer that I go ahead and do it?

Tom-Newton commented 3 years ago

I'm happy to make a PR but I don't know any way that I could test it.

JamesTimothyMeech commented 3 years ago

Ah I see what you mean. I'll make the change and test it sometime soon. Providing it doesn't break anything we can then make the PR and merge it in.