[LinuxPPS] refclock_nmea patch

[Rodolfo Giometti]

On Mon, Oct 22, 2007 at 06:12:45PM +0200, Udo van den Heuvel wrote:
> Rodolfo Giometti wrote:
> > 
> >> +		/* Try the alternare PPS device */
> >> +                (void) sprintf(device, DEVICEPPS, unit);
> >> +		msyslog(LOG_ERR, "refclock_nmea: try alternate PPS device %s", device);
> >> +                fd = open(device, O_RDWR);
> > 
> > Remoce the open, just use the time_pps_create() checking the return
> > value.
> 
> but time_pps_create needs a file handle which is created with opening
> the alternative device. (!?)

Ach! Sorry. I misread your code. You are completely right. :)

> I attached teh most recent patch with the suggested changes.
> 
> To ALL: Please have a look, please comment.
> If you're brave: please give it a try!

> --- /usr/src/redhat/ntp-4.2.4p2/ntpd/refclock_nmea.c	2006-06-06 22:16:53.000000000 +0200
> +++ refclock_nmea.c	2007-10-22 18:11:25.000000000 +0200
> @@ -61,6 +61,7 @@
>  # define DEVICE "COM%d:" 	/* COM 1 - 3 supported */
>  #else
>  # define DEVICE	"/dev/gps%d"	/* name of radio device */
> +# define DEVICEPPS "/dev/pps%d" /* name of alternate PPS radio device */
>  #endif
>  #define	SPEED232	B4800	/* uart speed (4800 bps) */
>  #define	PRECISION	(-9)	/* precision assumed (about 2 ms) */
> @@ -71,6 +72,7 @@
>  #define RANGEGATE	500000	/* range gate (ns) */
>  
>  #define LENNMEA		75	/* min timecode length */
> +#define LENPPS		PPS_MAX_NAME_LEN
>  
>  /*
>   * Tables to compute the ddd of year form icky dd/mm timecode. Viva la
> @@ -79,6 +81,8 @@
>  static int day1tab[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
>  static int day2tab[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
>  
> +char id[LENPPS] = "";
> +

I don't understand why you need it...

>  /*
>   * Unit control structure
>   */
> @@ -231,15 +235,28 @@
>  	 * the assert edge and do not enable the kernel hardpps.
>  	 */
>  	if (time_pps_create(fd, &up->handle) < 0) {
> -		up->handle = 0;
> -		msyslog(LOG_ERR,
> -		    "refclock_nmea: time_pps_create failed: %m");
> -		return (1);
> +		/* Try the alternare PPS device */
> +                (void) sprintf(device, DEVICEPPS, unit);
> +		msyslog(LOG_ERR, "refclock_nmea: try alternate PPS device %s", device);
> +                fd = open(device, O_RDWR);
> +                if (fd < 0)
> +                	goto pps_error;
> +                  
> +		if (time_pps_create(fd, &up->handle) < 0)
> +               		goto pps_error;
> +		msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\" at id #%d on \"%s\"", DEVICEPPS , fd, id);
>  	}
> +	else msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\" at id #%d on \"%s\"", DEVICE , fd, id);

Why don't you modify it as follow?

+             if (time_pps_create(fd, &up->handle) < 0)
+                             goto pps_error;
      }
+     else
+             (void) sprintf(device, DEVICE, unit); /* just rebuild device's name */
+     msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\"", device);

>  	return(nmea_ppsapi(peer, 0, 0));
> -#else
> +#else /* HAVE_PPSAPI */
>  	return (1);
>  #endif /* HAVE_PPSAPI */
> +
> +pps_error:
> +	/* No luck, no PPS unit available! */
> +	up->handle = -1;
> +	msyslog(LOG_ERR, "refclock_nmea: alternate PPS device %s fail : %m", device);

Better use:

+     msyslog(LOG_ERR, "refclock_nmea: no PPS devices found at " DEVICE " nor " DEVICEPPS ": %m", unit, unit);

> +	return (1);
>  }
>  
>  /*
> @@ -256,10 +273,7 @@
>  
>  	pp = peer->procptr;
>  	up = (struct nmeaunit *)pp->unitptr;
> -#ifdef HAVE_PPSAPI
> -	if (up->handle != 0)
> -		time_pps_destroy(up->handle);
> -#endif /* HAVE_PPSAPI */
> +	time_pps_destroy(up->handle);

Watch out. You need to protect time_pps_destroy(). Your patch should be:

 #ifdef HAVE_PPSAPI
-     if (up->handle != 0)
-             time_pps_destroy(up->handle);
+     time_pps_destroy(up->handle);
 #endif /* HAVE_PPSAPI */

>  	io_closeclock(&pp->io);
>  	free(up);
>  }
> @@ -366,7 +380,7 @@
>  	/*
>  	 * Convert the timespec nanoseconds field to ntp l_fp units.
>  	 */ 
> -	if (up->handle == 0)
> +	if (up->handle == -1)
>  		return (0);
>  	timeout.tv_sec = 0;
>  	timeout.tv_nsec = 0;

Ok, you are very near to the final patch! :)

-- 

GNU/Linux Solutions                  e-mail:    giometti at enneenne.com
Linux Device Driver                             giometti at gnudd.com
Embedded Systems                     		giometti at linux.it
UNIX programming                     phone:     +39 349 2432127