I would like to suggest not making it a compile time option but a runtime option.  This would get rid of the #ifdefs and be much more elegant.  You simply configure in the config file if you want to use gpsd or the raw device...

Well, it would be annoying to _have_to_ install gpsd just to compile cell_log ...

However, having gpsd shouldn't prevent device access if supported, just add new functions.

A few comments on the patch itself: - I would split the rename and adding the gpsd in two patches - You erroneously activated TX in your patch ... - You add whitespaces at the end of some lines.

(it's always a good idea to reread the patch you post. :)

Cheers,

Sylvain

On 02/09/2011 08:32 PM, Sylvain Munaut wrote:

...

I would like to suggest not making it a compile time option but a runtime option. This would get rid of the #ifdefs and be much more elegant. You simply configure in the config file if you want to use gpsd or the raw device...

Well, it would be annoying to _have_to_ install gpsd just to compile cell_log ...

Yes. That's why I used defines. If you all give me a definitive answer I can modify the patch in order to fulfil the needs. My opinion is that if someone doesn't have gps or doesn't want to use gpsd, the libgps dependency would be bad.

However, having gpsd shouldn't prevent device access if supported, just add new functions.

A few comments on the patch itself:

• I would split the rename and adding the gpsd in two patches

ok, I can do it.

• You erroneously activated TX in your patch ...
• You add whitespaces at the end of some lines.

(it's always a good idea to reread the patch you post. :)

You are right :), but I sent it for commenting. When sending the definitive I will do all the needed checks.

Hi guys Any news this side? Are patches ok? Are you planning to merge them? If not, tell me what's wrong so I can change it. Bye Dario.

On Thu, Feb 10, 2011 at 2:29 PM, Dario Lombardo dario.lombardo@libero.itwrote:

These are the 2 patches for renaming functions and gpsd support throught conditional compilation. Dario.

On Tue, Feb 15, 2011 at 05:37:48PM +0100, Dario Lombardo wrote:

Hi guys Any news this side? Are patches ok? Are you planning to merge them? If not, tell me what's wrong so I can change it.

Andreas said he is OK with applying them, so I will do that.

However, if I read your code correctly, it still seems to me that there is a fixed compile time decision if gpsd or built-in gps support is to be used.

I think it would be better to keep it a runtime decision, i.e.

1) if gpsd headers/library available during compilation, build support for _both_ gpsd and built-in gps into the program, 2) if they are not available, only include the built-in gps support.

The decision which method to use should be a config file option. Please make sure that a config file configured for built-in gps support will work with both versions of the program.

A config file requesting the use of gpsd support should make the program abort if it was compiled without gpsd support included.

So I will apply your patch now, but I would really appreciate if you could improve it the way I described in an incremental patch at some later point.

Thanks, Harald