nmrossomando
commented
8 years ago I have a number of concerns with this code base as is.
- Code size: As is, Balloonduino.h blindly includes multiple Adafruit libraries. Even on a Mega, that's a lot for an embedded system. Which libs are used should be up to the user through a series of
#definemacros. (I.e.#define USE_BME280in their .ino file would trigger an#ifdefblock in this code that included and initialized that sensor.) -RAM usage: This sorta ties in with code size. As is, this library instantiates an object for every sensor that code has been written for. If the user doesn't want them there, that's a waste of RAM on a system that has just 8 K of it. As an example, the BNO library alone eats 1% of the Mega's RAM while just sitting there doing nothing, as does the BME library. That may not seem like a lot, but it's still RAM being essentially wasted. This can easily be resolved with the above#definesystem, as well, but it's worth noting as a separate issue. - Sensor functionality obfuscation: By hiding the sensor libraries behind a class, the user is stuck with what it provides them. What if they want to put the device into sleep mode? What if they want quaternions instead of Euler angles? We can't write wrapper functions for all those things - that compounds the heck out of the code size issue. The user should have direct access to the libraries if they want it. (The "if they want it" part is tricky, and I'm not sure what the best way to go about that in particular would be yet. I'm beginning to think C-style straight functions in a Balloonduino namespace will be a better solution than an OOP object.)
- Minimal commenting: I'm reading this and I don't know why some things are happening. Example: There are unlabelled pins being pulled low and high. Why? (Macros for pin names work well here! Other things will probably need real comments.)
- Ambiguous naming: What status bits does
getStatusString()return? Also, in another commit, you mentionedBalloonduino::print(), which will be confused withSerial.print(), even though it writes to the SD card. Users are going to want that to be obvious from the name of the function rather than having to crawl through this code to find out. Everything we write here should have a painfully explicit naming convention:getSensorStatusString()orwriteToLogger(), for example, are better function names. - One actual code error I caught: All of our sensors use I2C, which uses two pins: SDA and SCL. The BME280 CS, MOSI, MISO, and SCK pin definitions aren't needed. Since they're totally unused macros, this is harmless, but it's confusing to someone else reading the code. Does the Adafruit library require these defines? If not, it's totally safe to remove them.
I hope I've done a good job explaining the issues I have, and didn't come across as too harsh. Embedded development is a tough paradigm to get used to, especially when writing code that other people will need to use without having to understand it very much. I'm glad you're working on this without prompting from me, however, and I'm as usual open to any questions or concerns.
zacharyburnett
Bring master up to date with development, to start work on SD card logging and XBee support.