Go Down

Topic: Seeking constructive criticism on first Arduino library (Read 971 times) previous topic - next topic

jgoulder

Hello, all.

I am new to the world of Arduino.  I picked one up this weekend and have been having fun playing with it.  I decided to create my own library for the MAX517 8-bit DAC.

Since I have never done this before, I was hoping to get some constructive criticism on the libraries' implementation.

It is hosted on GitHub: https://github.com/JordanGoulder/Max517Dac

Thanks for the help!

dc42

Hi,

Your code looks generally good to me. You have declared a class so that you can support multiple instances with different addresses (if that is possible for that device), which is good. I have just a few minor comments:

1. The static storage class on the declarations of constants CMD_RESET etc. is redundant. In C++ (although not in C), non-local constant declarations have file scope unless they are declared extern.

2. You are using type int everywhere, however type uint8_t or byte is more appropriate and more efficient for parameters and constants that represent 8-bit values to be transmitted or received. This applies to constants like CMD_RESET and parameters like the ones to mSendCommand.

3. You don't actually need to indicate an empty parameter list using (void), that is how you do it in C. In C++ it is sufficient and more usual to use just () instead. But it really doesn't matter.

4. You're making the assumption that either the user will never want to call setOutput with a bit pattern of all 1s, or that if he does, he will pass it as value 255 not -1. Better I think to make the parameter uint8_t or byte so that this issue doesn't arise.
Formal verification of safety-critical software, software development, and electronic design and prototyping. See http://www.eschertech.com. Please do not ask for unpaid help via PM, use the forum.

jgoulder

Thanks for the very helpful advice dc42.  I have made changes to fix this issues that you mentioned.

Go Up