search

kidoman / embd

Embedded Programming Framework in Go
http://embd.kidoman.io
MIT License
1.28k stars 156 forks source link

sensor: added driver for MCP9808 temperature sensor #71

Open alittlebrighter opened 7 years ago

alittlebrighter commented 7 years ago

I implemented this driver based off of documentation found here: MCP9808 Datasheet It mostly works but seems to hang after trying to write the the configuration register multiple times. Could anyone point me to how best to debug where things might be hanging in the I2C interface? Aside from that issue, does anything need changed in my driver code?

tve commented 7 years ago

Thanks for the PR! Looks nicely done overall :-). I think there are some issue I pointed out and it would be better IMHO if all functions went straight to the device as opposed to making some return info from memory.

alittlebrighter commented 7 years ago

Thanks for the review. Most of my work doesn't involve bit-twiddling so I appreciate you taking the time to correct some of my missteps in that regard. I'll get those changes made and see what you think.

In regards to the Set functions on config parameters since it didn't seem like I could write just a single bit to the device I thought it would be easier to make all of your config changes with a local copy and write the entire config word to the device as a separate step saving time making writes over the I2C bus. If that doesn't make sense I can change it so all of the getters and setters go straight to the device.

tve commented 7 years ago

I'm not sure you caught the likely cause of your deadlock (see comment for line 442).

I do think that making the set function go straight to the device would be more intuitive. As far as I can tell, everything pretty much gets set once and that's it, it's not like those changes happen frequently or are in some critical section. For this reason I would prioritize ease of use over performance.

alittlebrighter commented 7 years ago

I had a crazy weekend but I believe I've made all of the fixes you suggested and fixed an issue with temperature reading/writing that I had missed in the documentation. Let me know if this version looks better.

I still can't get the interrupt to work although the comparators work just fine.

alittlebrighter commented 7 years ago

I'll have to study that code some more and read up on two's complement but thanks for the examples. It should be good to go now.

tve commented 7 years ago

did the "hangs in the I2C bus" issue get fixed?

alittlebrighter commented 7 years ago

Yes, that was just my own stupidity with the locking. Also, I just fixed the rounding issue. Per section 5.1.2 in the documentation the limit registers are only precise to a quarter of a degree Celsius hence I changed the mask to 0x1ffc and per some testing here adding two gets the closest to accurate rounding for both positive (exact) and negative (off by a little) values.

tve commented 7 years ago

LGTM, are there more change you'd like to make?

alittlebrighter commented 7 years ago

Those last two commits are it.

alittlebrighter commented 7 years ago

Is there anything else I need to do on this?