REQUEST FOR COMMENT make arduino optional

[close2]

this branch tries to make the arduino platform optional, so that the library may be used on other platforms.

Is this something you would even consider merging?

Note that I haven't yet tested anything, but if there is no chance of this branch being merged I would rather just copy the code I need instead of working on the fork.

DO NOT MERGE (YET)

[SlashDevin]

Are you saying NULL is not defined on your platform? Wow.

Please try nullptr instead. This is a newer language feature intended to mean "a pointer to nothing", not just a #define to zero.

[SlashDevin]

this branch tries to make the arduino platform optional, so that the library may be used on other platforms.

Is this something you would even consider merging?

Yes, actually. It has been a while since I tried to minimize the includes, so some of the differences might go away.

As you have discovered, the biggest change is the use of Print and Stream. I would want to know if there are output and input stream classes that you would use. Perhaps shim classes or set of #defines could be used, or maybe the related methods should become a "facet" that just isn't available for your platform.

[close2]

Hi @SlashDevin

I have (partially) adapted your library so that it might be used for other frameworks / systems.

I guess you have to decide how you want to see your library. Should it stay an arduino library which might also be used in other systems. Or do you want this library to be a "standalone" library which can be used in an arduino.

My current approach is more in line with the first version.

IMO the biggest part missing is some kind of build system support (cmake?) for other platforms.
I am also don't know where to put the files.

If you like this approach I will resolve merge conflicts.

[SlashDevin]

I guess you have to decide how you want to see your library. Should it stay an arduino library which might also be used in other systems. Or do you want this library to be a "standalone" library which can be used in an arduino.

My current approach is more in line with the first version

Well... I always prefer a platform-independent approach, with the minimum number of "connections" to a specific platform. I think that's your second choice. I think NeoGPS is platform-dependent in its use of FLASH memory storage specifiers and the output functions (Print and for pathological reasons, Stream). If I had tried to use it under Windows, I think I would have tried to "disconnect" most of these. The storage specifiers are pretty easy to #define or typedef away, but the output functions are a little too coupled, IMHO.

I think I'm having trouble with the templates for Stream and Print. You have matched the Arduino Print interface with linux versions (what I call a shim), so why do you also need a template in the NMEAGPS class? It looks like NMEAGPS could just use your linux platform classes.

I wonder if it might be better to switch NeoGPS output functions to use the streaming operator on a platform-specific output stream. For example:

#ifdef ARDUINO
  #define NEOGPS_OSTREAM Print
#else // if defined(LINUX) ???
  #define NEOGPS_OSTREAM std::ostream
#endif

class NMEAGPS
{
  void send_P( NEOGPS_OSTREAM & os, const __FlashStringHelper *msg );
};


void NMEAGPS::send_P( NEOGPS_OSTREAM & os, const __FlashStringHelper *msg )
{
  if (msg) {
    const char *ptr = (const char *)msg;
    char  chr = pgm_read_byte(ptr++);

    os << '$';
    if (chr == '$')
      chr = pgm_read_byte(ptr++);
    uint8_t sent_trailer = 0;
    uint8_t crc          = 0;
    while (chr) {
      if ((chr == '*') || (sent_trailer > 0))
        sent_trailer++;	
      else
        crc ^= chr;
      os << chr;

      chr = pgm_read_byte(ptr++);
    }

    if (!sent_trailer)
      send_trailer( os, crc );
   }
 
 } // send_P

This is perhaps more portable than requiring the shim classes to be written for each platform.

Also, I have never like how template implementations must go in the headers. I understand why, but it has a significant impact on readability.

I have not considered how the Arduino examples (INO extension, egad) might also be platform-independent. Perhaps the LINUX examples (i.e., main) could set some globals (an std::iostream for the console [aka DEBUG_PORT], an std::istream for the GPS [aka gpsPort]), call setup and then loop.

This is a good exercise!

[close2]

Why not put the implementation into another header file: NMEAGPS.impl.h

Personally I would remove all cpp files. This library will probably always be used in situations where compile time is not an issue.
In addition unless you compile with either link-time-optimization (-flto) or the whole-program optimization (-fwhole-program) it will also produce better / faster executables.

I started my implementation with shims. However this meant that even if I didn't use any print statements a shim implementation had to be present.

By making those function template functions they are only compiled when used and the corresponding shims don't need to be implemented.

I think that we should never do something like:

#ifdef ARDUINO
  #define NEOGPS_OSTREAM Print
#else // if defined(LINUX) ???
  #define NEOGPS_OSTREAM std::ostream
#endif

For instance my original intention was to use NeoGPS on an stm32.
Should I then modify NeoGPS and add another #ifdef?

We should at least add another guard:

#ifndef NEOGPS_OSTREAM
... as before
#endif

I would also move those Platform dependent #ifdefs into one location. (I moved already a lot of stuff into Platform.h.)

OT: you mentioned that you don't like the #ifdefs to enable / disable functionality. Have you ever tried if a modern compiler will even generate that code if you don't read the values? I would assume that virtually every optimizing compiler will not set values unless you read them at some point.

[SlashDevin]

Why not put the implementation into another header file: NMEAGPS.impl.h

Personally I would remove all cpp files.

LOL, just rename all the .cpp files to .impl.h files... I'm ok with that.

unless you compile with either link-time-optimization (-flto) or the whole-program optimization (-fwhole-program)...

Are you saying you don't do that on Linux? That's the default on the Arduino, so I have always been able to confirm that unused methods are removed from the executable image. Since compile time is not an issue, why would you not do this?

I think that we should never do something like:

#ifdef ARDUINO<br>
 #define NEOGPS_OSTREAM Print<br>
#else // if defined(LINUX) ???<br>
 #define NEOGPS_OSTREAM std::ostream<br>
#endif<br>

...I would also move those Platform dependent #ifdefs into one location. (I moved already a lot of stuff into Platform.h.)

Yes, definitely. Bad example. Two different Platform.h files should contain the two different #define statements.

I started my implementation with shims. However this meant that even if I didn't use any print statements a shim implementation had to be present.

By making those function template functions they are only compiled when used and the corresponding shims don't need to be implemented.

Interesting! I'm not very familiar with templates, so it never occurred to me that the compiler would "ignore" a template implementation where the template argument is an undefined class. Kinda cool, kinda half-way to a preprocessor macro. I had to look closer at which files really include platforms/Print.h.

I think I don't mind requiring a shim, because the default could be do-nothing. For new platforms, that would serve as an implementation starting point. That would also remove the template noise from the header, and surely all platforms would either remove do-nothing code (the default shim) or remove unreferenced code (shim provided but never used). Surely?

OT: you mentioned that you don't like the #ifdefs to enable / disable functionality. Have you ever tried if a modern compiler will even generate that code if you don't read the values? I would assume that virtually every optimizing compiler will not set values unless you read them at some point.

Hmm, I'm not sure what you mean by "read the values".

In general, I don't like the noise of preprocessor directives mixed in with code. When I can, I have started using #define to set a C++ constant value . Then I use that constant in C++ code. That constant value can be evaluated a compile time, leading to the same optimizations as #if conditionals, but without the noise (e.g., see NMEAGPS_PROCESSING_STYLE).

I'm ok with #defines for constant values (e.g., NMEAGPS_FIX_MAX), because using them is not noisy. I probably need to revisit those to see if there is a better static const approach.

I also don't like that I have to use conditionals for optional class/structure members (e.g., gps_fix), both in the declaration and when using them. To my knowledge, the class/structure size will not be optimized (reduced) if a member is never used (assuming that the member usage is still conditional). I've been toying with other techniques for the gps_fix structure. Would you like to suggest a template solution for this? Not that angle brackets, template arguments and cryptic compile error messages would be more readable... ;)

What is your opinion about using the streaming operators to provide some of the platform independence? Perhaps a shim/template isn't needed if all Print & Stream uses are restricted to << and >>? For eliminating the reference to available...

I am also thinking about how the Arduino sketches have to poll continuously because there is no OS, while Linux programs can just use a blocking read. There's something irritating about requiring Linux programs to poll via available(), but I can't put my finger on it. NMEAGPS::handle might actually be NMEAGPS::operator <<, and some of the print methods might be operator >>. Some minor changes in NMEAGPS and bigger changes to the examples could rectify this.

[close2]

unless you compile with either link-time-optimization (-flto) or the whole-program optimization (-fwhole-program)...
Are you saying you don't do that on Linux?

I do now. But you have to know that those flags exist. I've never used the Arduino platform and for instance avr-libc doesn't mention those flags. The first search result for 'avr makefile' is avr-libc.

Also -flto is not as powerful as -fwhole-program. And when I started writing AVR programs, those flags didn't even exist...

And not having any .cpp files is a good "work-around" those problems ;)

I think I don't mind requiring a shim,

Well, then shims it is.

Hmm, I'm not sure what you mean by "read the values".

If a compiler can remove variables without the programmer noting a difference it usually will. So even if you write to a variable, but then never read from it, the compiler will remove the whole code, which writes to the variable.

I have to admit, that this is not something easily verified and even taking the pointer of a variable might force the compiler to add unused variables again.

#include <avr/io.h>
  
class A {
        private:
                long a;
                long b;
        public:
                long getA() { return a; }
                void setA(long x) { a = x; }

                long getB() { return b; }
                void setB(long x) { b = x; }
};

int main() {
        uint8_t x1 = PORTC;
        uint8_t x2 = PORTC;
        uint8_t x3 = PORTC;
        uint8_t x4 = PORTC;

        A as[2];
        as[0].setA(x1 + 0x0F);
        as[1].setA(x2 + 0x0F);
        as[0].setB(x3 + 0x0F);
        as[1].setB(x4 + 0x0F);

        uint8_t x = PORTC;

        if (x > 0) {
                PORTB = x + ((uint8_t)as[0].getA());
        } else {
                PORTB = x + ((uint8_t)as[1].getB());
        }

        for(;;);
}

with avr-g++ -g -O3 -fwhole-program -Wall -mmcu=atmega48a main.cpp
and avr-objdump -d a.out
will produce:

; [...]
00000046 <main>:
  46:   88 b1           in      r24, 0x08       ; 8
  48:   98 b1           in      r25, 0x08       ; 8
  4a:   98 b1           in      r25, 0x08       ; 8
  4c:   98 b1           in      r25, 0x08       ; 8
  4e:   28 b1           in      r18, 0x08       ; 8
  50:   22 23           and     r18, r18
  52:   21 f0           breq    .+8             ; 0x5c <main+0x16>
  54:   82 0f           add     r24, r18
  56:   81 5f           subi    r24, 0xF1       ; 241
  58:   85 b9           out     0x05, r24       ; 5
  5a:   ff cf           rjmp    .-2             ; 0x5a <main+0x14>
  5c:   91 5f           subi    r25, 0xF1       ; 241
  5e:   95 b9           out     0x05, r25       ; 5
  60:   fc cf           rjmp    .-8             ; 0x5a <main+0x14>
; [...]

Note that there are not even any variables any longer. And r25 is overwritten twice as the value is not needed.

However adding those 2 lines directly before for(;;);:

        PORTC = (long)&as[0] & 0xFF;
        PORTC = (long)&as[1] & 0xFF;

will increase the asm code of the main function to 74 lines and put the objects into ram.

I don't think that all #ifdefs can be removed, but for simple cases the compiler might be good enough.

[SlashDevin]

I'm not sure what you mean by "read the values".

If a compiler can remove variables without the programmer noting a difference it usually will. So even if you write to a variable, but then never read from it, the compiler will remove the whole code, which writes to the variable.

I have to admit, that this is not something easily verified

Yes, I have spent some time reviewing the optimization of variables. I wondered if you thought that data members would be optimized out of a class/structure if they are never referenced. Optional member declaration and usage must remain guarded by #ifdef.     :(

shims it is.

I keep wondering how much Print/Stream shim is actually required:

What is your opinion about using the streaming operators to provide some of the platform independence? Perhaps a shim/template isn't needed if all Print & Stream uses are restricted to << and >>?

If Platform.h makes operator << visible for the NEOGPS_OSTREAM type, you don't need a shim for Print. Ditto NEOGPS_ISTREAM, except for the pesky available method... which took me down the rabbit hole of polling vs. blocked.

[close2]

Hello @SlashDevin

I've spent some time to modify my PR.

Platform dependent code is now included through Platform.h

There are 3 classes which should be implemented:
Stream, Print and System (see the corresponding files in platform/)

I've added a tests order (which currently doesn't test anything) showing how to use a fakeGPS and stdout.

[close2]

this function is not necessary and should be removed.

uint8_t is usually typedef'd to unsigned char and for instance std::cout will not print a number but the corresponding ascii char.

I've added a + in front of all uint8_t prints which automatically casts them them to int.

[SlashDevin]

std::cout prints the ASCII character for unsigned char? How are you supposed to print the number? unsigned short? Cast it to int? Hmmm...

[close2]

Yes. You need to cast.
The easiest way to cast is by adding + in front of an uint8_t
std::cout << n; becomes std::cout << +n; This will cast uint8_t to ints. All other types stay the same. I've added a + in front of all << aNumber. Regardless of the actual number type. All numbers are now output as outs << +aNumber;

[SlashDevin]

Doh! Now your previous comment makes sense:

I've added a + in front of all uint8_t prints which automatically casts them them to int.

I was totally derailed by the source code diffs above that show "+" at the beginning of the line...

[SlashDevin]

It will take me a while to go through all of this, and coordinate it with my works in progress. Thanks!

[SlashDevin]

Just a note to let you know I'm still working on this... I'm investigating the Windows platform, too.