melexis / mlx90640-library

MLX90640 library functions
Apache License 2.0
241 stars 192 forks source link

Typos in driver doc #1

Closed nseidle closed 6 years ago

nseidle commented 6 years ago

Hi,

I have the MLX90640 outputting data but have not yet gotten the driver to work.

I have read over the driver doc and the datasheet for the MLX90640 and found many typos and confusing errors. I would like to improve the documents. If I propose fixes, will you be able to correct the doc? How would you like typo fixes versus technical change recommendations?

A trivial example: page 10

MLX90640_ExtractParameters(eeMLX90640, &mlx90640);

is correct but every following page

MLX90640_ExtractParameters(eeMLX90640,mlx90640);

Another, page 11:

vdd = GetVddMLX90640(mlx90640Frame, &mlx90640); //vdd = 3.3

should be

vdd = MLX90640_GetVddMLX90640(mlx90640Frame, &mlx90640); //vdd = 3.3

Cheers, -Nathan

slavysis commented 6 years ago

Hi,

Any feedback and recommendations are more than welcome. I will gladly update the document. I would very much like to make this driver easy and fast to implement. I have already fixed the typos you mention and am looking forward to any suggestions that would make the driver and/or the document better.

Best regards

nseidle commented 6 years ago

I got the driver to work this weekend. It's extremely helpful. Thank you for writing it!

Another typo:

One of the hiccups I had implementing the driver was related to typedef of the I2C functions. Mainly this one:

int MLX90640_I2CRead(uint8_t slaveAddr, unsigned int startAddress, unsigned int nWordsRead, uint16_t *data) 

It was originally unclear what nWordsRead was intended for. A word can mean different things on different platforms. It took me a few tries to figure out the function should be reading 16-bits per EEPROM location not 32-bits. I believe the read/write functions could benefit from tighter typedef'ing as well.

I recommend changing to:

int MLX90640_I2CWrite (uint8_t slaveAddr, uint16_t writeAddress, uint16_t data) 
int MLX90640_I2CRead(uint8_t slaveAddr, uint16_t startAddress, uint16_t nEEPROMLocationsRead, uint16_t *data) 

This would force me to read about the EEPROM locations and realize the EEPROM is 16-bits wide.

slavysis commented 6 years ago

Normally it should not be an issue as nWordsRead is used as an index, but I do agree that it is better to have it defined more strictly and it is done. The name nWordRead is changed to nMemAddressRead as this is a more general purpose function and it is not called for reading just the EEPROM, but also other memory locations. I really appreciate your feedback and am looking forward to other fixes that could be made.

Best regards

nseidle commented 6 years ago

Sounds good to me. Thank you so much for making these changes!