adafruit / Adafruit_GPS

An interrupt-based GPS Arduino library for no-parsing-required use
Other
475 stars 318 forks source link

Fix a few warnings #112

Closed optorres closed 4 years ago

optorres commented 4 years ago

This fixes the following warnings:

sellensr commented 4 years ago

The constants you made explicitly float won't cause any loss of precision if nmea_float_t (https://github.com/adafruit/Adafruit_GPS/pull/111) is set to double, as they are round numbers.

I will add the double casts on pending additions to build(). It can readily be eliminated by hand, along with other possibly extraneous functionality, by killing the definition of NMEA_EXTENSIONS in Adafruit_GPS.h, even if the optimizer doesn't eliminate it.

sellensr commented 4 years ago

Not sure this will still be mergeable if #111 is merged first.

optorres commented 4 years ago

Yes, I'd expect some merge conflicts, but they shouldn't be too hard to resolve. The main issue would be the switch to double, but I see you've reverted that. I can rebase this if/when #111 is merged.

sellensr commented 4 years ago

I've been flipping back and forth on double/float. The lack of float precision is how we got to having both float and integer fixed versions of position. Given the arrival rates of order 10 Hz, I don't think FPU speed is going to be an issue inside the library, unless people are using the functions for other processing. Switching to all double eliminates the precision issue and only adds a few hundred bytes of storage, even with the additional NMEA values I hope to manage. That should only be an issue on the smallest processors and AVR downgrades doubles to float automatically. I can go either way on default and nmea_float_t enables user customization.

optorres commented 4 years ago

I'm OK with it being configurable via a define or similar. I don't want to have to modify the library files to change the precision though, since then I have to distribute a modified copy with my project instead of just adding it to lib_deps.

sellensr commented 4 years ago

Not sure how to inject a define into the compiling of the library... any ideas?

optorres commented 4 years ago

You could do something like this.

#ifndef NMEA_FLOAT_T
#define NMEA_FLOAT_T float
#endif
typedef NMEA_FLOAT_T nmea_float_t;

Then you can add -DNMEA_FLOAT_T=double to the CFLAGS for the project.

sellensr commented 4 years ago

Thanks optorres. I'm just not sure how to add a command line flag to a project where the Arduino IDE is doing all the work?

optorres commented 4 years ago

Hmmm, it looks like the Arduino IDE doesn't have a compiler flags option. In PlatformIO you can just use the build_flags option. Unfortunately it seems like there's no way to configure libraries with the Arduino IDE other than copying them into your project and modifying the source files directly, so using a define would just make things easier for those that want to change the option when using PlatformIO, Makefiles, or another build system.

sellensr commented 4 years ago

I see hints that it may be possible, but hard to find. I'll build in the define so flags can be used under PlatformIO.

sellensr commented 4 years ago

https://ecampusontario.pressbooks.pub/rwsnotes/back-matter/advanced-arduino-ide-configuration in case you were wondering if I found something ;-)

optorres commented 4 years ago

Rebased.