Discussion:
[Linuxptp-devel] [PATCH 0/5] Add configuration file support to phc2sys
Jacob Keller
2015-01-10 00:16:28 UTC
Permalink
This series adds phc2sys configuration file support. Right now all the
command line options are supported. Like ptp4l, any existing command
line arguments supersede configuration file. Also, while the generic
options which appear to be simlar for both ptp4l and phc2sys are read
from [global], all options can be specified in [phc2sys] overriding
settings from [global] (but not the command line).

I didn't think the effort necessary to re-write the configuration such
that it was more generic was worth it. Thus, there is some level of code
duplication as phc2sys does not need the same configuration values as
ptp4l.

I did update the manuals, and have done some minor (compile and a few
run time checks) testing. However currently this series is more of an
RFC.

The intention is to enable additional non-command line options for
phc2sys. We could even deprecate several current phc2sys options which
are no longer necessary (similar to ptp4l which has many config options
but few command line parameters).

In addition this opens the door to bring several settings from ptp4l
into phc2sys (like the PI servo settings).

Jacob Keller (5):
util: move some config code into util for sharing
config: add phc2sys section ignored by ptp4l
ptpl4.8: update ptp4l man page to indicate the phc2sys section
config: add global options check function
phc2sys: support reading the configuration file

config.c | 147 +++++++++++---------
config.h | 2 +
makefile | 2 +-
phc2sys.8 | 174 +++++++++++++++++++++++-
phc2sys.c | 454 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
ptp4l.8 | 21 ++-
util.c | 62 +++++++++
util.h | 43 ++++++
8 files changed, 834 insertions(+), 71 deletions(-)
--
2.1.2.555.gfbecd99
Jacob Keller
2015-01-10 00:16:29 UTC
Permalink
Move some various utility functions used by the config parser into
util.c

The main reason for this is to help extend phc2sys to read a
configuration file as well. It is easier to only use the generic
functions, as phc2sys does not use the exact same configuration settings
or structure as ptp4l.

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 65 ----------------------------------------------------------------
util.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
util.h | 42 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 65 deletions(-)

diff --git a/config.c b/config.c
index ec8c53801af0..7e783f3f7f88 100644
--- a/config.c
+++ b/config.c
@@ -27,30 +27,6 @@
#include "print.h"
#include "util.h"

-enum config_section {
- GLOBAL_SECTION,
- PORT_SECTION,
- UNKNOWN_SECTION,
-};
-
-static enum parser_result parse_section_line(char *s, enum config_section *section)
-{
- if (!strcasecmp(s, "[global]")) {
- *section = GLOBAL_SECTION;
- } else if (s[0] == '[') {
- char c;
- *section = PORT_SECTION;
- /* Replace square brackets with white space. */
- while (0 != (c = *s)) {
- if (c == '[' || c == ']')
- *s = ' ';
- s++;
- }
- } else
- return NOT_PARSED;
- return PARSED_OK;
-}
-
static enum parser_result parse_pod_setting(const char *option,
const char *value,
struct port_defaults *pod)
@@ -592,47 +568,6 @@ static enum parser_result parse_global_setting(const char *option,
return PARSED_OK;
}

-static enum parser_result parse_setting_line(char *line,
- const char **option,
- const char **value)
-{
- *option = line;
-
- while (!isspace(line[0])) {
- if (line[0] == '\0')
- return NOT_PARSED;
- line++;
- }
-
- while (isspace(line[0])) {
- line[0] = '\0';
- line++;
- }
-
- *value = line;
-
- return PARSED_OK;
-}
-
-static void check_deprecated_options(const char **option)
-{
- const char *new_option = NULL;
-
- if (!strcmp(*option, "pi_offset_const")) {
- new_option = "step_threshold";
- } else if (!strcmp(*option, "pi_f_offset_const")) {
- new_option = "first_step_threshold";
- } else if (!strcmp(*option, "pi_max_frequency")) {
- new_option = "max_frequency";
- }
-
- if (new_option) {
- fprintf(stderr, "option %s is deprecated, please use %s instead\n",
- *option, new_option);
- *option = new_option;
- }
-}
-
int config_read(char *name, struct config *cfg)
{
enum config_section current_section = UNKNOWN_SECTION;
diff --git a/util.c b/util.c
index 06c3296cf189..861d1269bdd9 100644
--- a/util.c
+++ b/util.c
@@ -22,6 +22,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <ctype.h>

#include "address.h"
#include "print.h"
@@ -216,6 +217,65 @@ int leap_second_status(uint64_t ts, int leap_set, int *leap, int *utc_offset)
return leap_status;
}

+enum parser_result parse_section_line(char *s, enum config_section *section)
+{
+ if (!strcasecmp(s, "[global]")) {
+ *section = GLOBAL_SECTION;
+ } else if (s[0] == '[') {
+ char c;
+ *section = PORT_SECTION;
+ /* Replace square brackets with white space. */
+ while (0 != (c = *s)) {
+ if (c == '[' || c == ']')
+ *s = ' ';
+ s++;
+ }
+ } else
+ return NOT_PARSED;
+ return PARSED_OK;
+}
+
+enum parser_result parse_setting_line(char *line,
+ const char **option,
+ const char **value)
+{
+ *option = line;
+
+ while (!isspace(line[0])) {
+ if (line[0] == '\0')
+ return NOT_PARSED;
+ line++;
+ }
+
+ while (isspace(line[0])) {
+ line[0] = '\0';
+ line++;
+ }
+
+ *value = line;
+
+ return PARSED_OK;
+}
+
+void check_deprecated_options(const char **option)
+{
+ const char *new_option = NULL;
+
+ if (!strcmp(*option, "pi_offset_const")) {
+ new_option = "step_threshold";
+ } else if (!strcmp(*option, "pi_f_offset_const")) {
+ new_option = "first_step_threshold";
+ } else if (!strcmp(*option, "pi_max_frequency")) {
+ new_option = "max_frequency";
+ }
+
+ if (new_option) {
+ fprintf(stderr, "option %s is deprecated, please use %s instead\n",
+ *option, new_option);
+ *option = new_option;
+ }
+}
+
enum parser_result get_ranged_int(const char *str_val, int *result,
int min, int max)
{
diff --git a/util.h b/util.h
index 98b395bce329..3f2863899b3d 100644
--- a/util.h
+++ b/util.h
@@ -136,6 +136,48 @@ enum parser_result {
};

/**
+ * Configuration sections used in the configuration file
+ */
+enum config_section {
+ GLOBAL_SECTION,
+ PORT_SECTION,
+ UNKNOWN_SECTION,
+};
+
+/**
+ * Parse config line for section heading
+ *
+ * @param s String value line from configuration file
+ * @param section the current section, modified if line is a section header
+ *
+ * @return PARSED_OK on successful section found, otherwise returns
+ * NOT_PARSED for any non section line.
+ */
+enum parser_result parse_section_line(char *s, enum config_section *section);
+
+/**
+ * Parse a configuration line into option and value
+ *
+ * @line String value line from configuration file
+ * @option Contains option on return
+ * @value Contains option value on return
+ *
+ * @return PARSE_OK if line can be split into option and value,
+ * otherwise returns NOT_PARSED
+ */
+enum parser_result parse_setting_line(char *line,
+ const char **option,
+ const char **value);
+
+/**
+ * Convert deprecated option into new value, and warn user
+ *
+ * @option Converted to new option name on return if matched a
+ * deprecated option
+ */
+void check_deprecated_options(const char **option);
+
+/**
* Get an integer value from string with error checking and range
* specification.
*
--
2.1.2.555.gfbecd99
Jacob Keller
2015-01-10 00:16:30 UTC
Permalink
Add a new configuration section phc2sys which is ignored by ptp4l. It
may be worth adding a "port:" prefix to all per-port configuration in
the configuration file (and deprecate the old syntax) in order to allow
full expression of subsections and port names. Currently it will not be
possible for a network device name to match a subsection name. This is
not a large problem at present, however.

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 2 ++
util.c | 2 ++
util.h | 1 +
3 files changed, 5 insertions(+)

diff --git a/config.c b/config.c
index 7e783f3f7f88..74c9e5fbccda 100644
--- a/config.c
+++ b/config.c
@@ -664,6 +664,8 @@ int config_read(char *name, struct config *cfg)
case UNKNOWN_SECTION:
fprintf(stderr, "line %d is not in a section\n", line_num);
goto parse_error;
+ /* ignore phc2sys specific configuration */
+ case PHC2SYS_SECTION:
default:
continue;
}
diff --git a/util.c b/util.c
index 861d1269bdd9..1991892b36c2 100644
--- a/util.c
+++ b/util.c
@@ -221,6 +221,8 @@ enum parser_result parse_section_line(char *s, enum config_section *section)
{
if (!strcasecmp(s, "[global]")) {
*section = GLOBAL_SECTION;
+ } else if (!strcasecmp(s, "[phc2sys]")) {
+ *section = PHC2SYS_SECTION;
} else if (s[0] == '[') {
char c;
*section = PORT_SECTION;
diff --git a/util.h b/util.h
index 3f2863899b3d..ac5eb89c655e 100644
--- a/util.h
+++ b/util.h
@@ -141,6 +141,7 @@ enum parser_result {
enum config_section {
GLOBAL_SECTION,
PORT_SECTION,
+ PHC2SYS_SECTION,
UNKNOWN_SECTION,
};
--
2.1.2.555.gfbecd99
Jacob Keller
2015-01-10 00:16:32 UTC
Permalink
This function is intended to be used by phc2sys in order to ignore (ie:
not warn) about options in the configuration file specific to ptp4l.
Since phc2sys reads from the global section as well as its own specific
section, it is important to ensure that phc2sys won't complain about
ptp4l options.

Add an array of known strings which all future global options must be
added to.

Signed-off-by: Jacob Keller <***@intel.com>
---
config.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
config.h | 2 ++
2 files changed, 92 insertions(+)

diff --git a/config.c b/config.c
index 74c9e5fbccda..ee5f251fa6c9 100644
--- a/config.c
+++ b/config.c
@@ -27,6 +27,96 @@
#include "print.h"
#include "util.h"

+static const char *all_global_options[] = {
+ "delayAsymmetry",
+ "logAnnounceInterval",
+ "logSyncInterval",
+ "logMinDelayReqInterval",
+ "logMinPdelayReqInterval",
+ "announceReceiptTimeout",
+ "syncReceiptTimeout",
+ "transportSpecific",
+ "path_trace_enabled",
+ "follow_up_info",
+ "neighborPropDelayThresh",
+ "min_neighbor_prop_delay",
+ "egressLatency",
+ "ingressLatency",
+ "fault_badpeernet_interval",
+ "fault_reset_interval",
+ "network_transport",
+ "delay_mechanism",
+ "delay_filter",
+ "delay_filter_length",
+ "boundary_clock_jbod",
+ "twoStepFlag",
+ "slaveOnly",
+ "gmCapable",
+ "priority1",
+ "priority2",
+ "domainNumber",
+ "clockClass",
+ "clockAccuracy",
+ "offsetScaledLogVariance",
+ "free_running",
+ "freq_est_interval",
+ "assume_two_step",
+ "tx_timestamp_timeout",
+ "check_fup_sync",
+ "pi_proportional_const",
+ "pi_integral_const",
+ "pi_proportional_scale",
+ "pi_proportional_exponent",
+ "pi_proportional_norm_max",
+ "pi_integral_scale",
+ "pi_integral_exponent",
+ "pi_integral_norm_max",
+ "step_threshold",
+ "first_step_threshold",
+ "max_frequency",
+ "sanity_freq_limit",
+ "ntpshm_segment",
+ "ptp_dst_mac",
+ "p2p_dst_mac",
+ "udp6_scope",
+ "uds_address",
+ "logging_level",
+ "verbose",
+ "use_syslog",
+ "time_stamping",
+ "delay_mechanism",
+ "network_transport",
+ "clock_servo",
+ "productDescription",
+ "revisionData",
+ "userDescription",
+ "manufacturerIdentity",
+ "summary_interval",
+ "kernel_leap",
+ "timeSource",
+ "delay_filter",
+ "delay_filter_length",
+ "boundary_clock_jbod",
+ /* NULL indicates end of options */
+ NULL,
+};
+
+/* used by phc2sys to ignore global options meant strictly for ptp4l */
+enum parser_result parse_known_global_options(const char *option)
+{
+ const char **item = &all_global_options[0];
+
+ while (*item != NULL) {
+ if (!strcmp(option, *item))
+ return PARSED_OK;
+
+ item++;
+ }
+
+ /* we didn't find option in the list */
+ return NOT_PARSED;
+}
+
static enum parser_result parse_pod_setting(const char *option,
const char *value,
struct port_defaults *pod)
diff --git a/config.h b/config.h
index c870e42ef162..99aa75435218 100644
--- a/config.h
+++ b/config.h
@@ -95,9 +95,11 @@ struct config {
int verbose;
};

+enum parser_result parse_known_global_options(const char *option);
int config_read(char *name, struct config *cfg);
struct interface *config_create_interface(char *name, struct config *cfg);
void config_init_interface(struct interface *iface, struct config *cfg);
void config_destroy(struct config *cfg);

+
#endif
--
2.1.2.555.gfbecd99
Keller, Jacob E
2015-01-12 22:05:17 UTC
Permalink
Post by Jacob Keller
+static const char *all_global_options[] = {
+ "delayAsymmetry",
+ "logAnnounceInterval",
+ "logSyncInterval",
...
Post by Jacob Keller
+ NULL,
+};
+
+/* used by phc2sys to ignore global options meant strictly for ptp4l */
+enum parser_result parse_known_global_options(const char *option)
+{
+ const char **item = &all_global_options[0];
+
+ while (*item != NULL) {
+ if (!strcmp(option, *item))
+ return PARSED_OK;
+
+ item++;
+ }
+
+ /* we didn't find option in the list */
+ return NOT_PARSED;
+}
So keeping lists of strings to ignore seems gross to me. It really
points to the fact that the config code needs some TLC. For example,
if we had the "struct config_item", then this could be implemented in
a straightforward, unified way, as a field in the struct.
Thanks,
Richard
This was just the most straight forward way, however....

Agreed, your outline of design seems a lot more straight forward, and
would resolve most of the ugly that is in this series (or pre-existing).

Regards,
Jake
Jacob Keller
2015-01-10 00:16:31 UTC
Permalink
This section currently does nothing, but is present for future phc2sys
configuration extension. Update the ptp4l manual in order to indicate
that the configuration file could be shared.

Note that the user is not required (obviously) to use the same file for
both ptp4l and phc2sys, though the goal is to make it possible to share
the configuration file if desired.

Signed-off-by: Jacob Keller <***@intel.com>
---
ptp4l.8 | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/ptp4l.8 b/ptp4l.8
index 0d01f29f4bda..fd5b1274bece 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -94,22 +94,29 @@ Display a help message.

.SH CONFIGURATION FILE

-The configuration file is divided into sections. Each section starts with a
-line containing its name enclosed in brackets and it follows with settings.
-Each setting is placed on a separate line, it contains the name of the
-option and the value separated by whitespace characters. Empty lines and lines
-starting with # are ignored.
+The configuration file is divided into sections and may be shared with phc2sys.
+Each section starts with a line containing its name enclosed in brackets and it
+follows with settings. Each setting is placed on a separate line, it contains
+the name of the option and the value separated by whitespace characters. Empty
+lines and lines starting with # are ignored.

The global section (indicated as
.BR [global] )
-sets the program options, clock options and default port options. Other
+sets the program options, clock options and default port options. Note that some options are shared with
+.B phc2sys.
+Other
sections are port specific sections and they override the default port options.
The name of the section is the name of the configured port (e.g.
.BR [eth0] ).
Ports specified in the configuration file don't need to be
specified by the
.B \-i
-option. An empty port section can be used to replace the command line option.
+option. An empty port section can be used to replace the command line option. The
+.BR [phc2sys]
+section is used for options specific to the
+.B phc2sys
+program and is outlined in phc2sys(8)
+

.SH PORT OPTIONS
--
2.1.2.555.gfbecd99
Jacob Keller
2015-01-10 00:16:33 UTC
Permalink
This commit enables phc2sys to read the configuration file. A new
phc2sys section is added for specific options. Those options which carry
over from ptp4l are read from both global and (with higher precedence)
phc2sys section. I did not go to the extreme effort of refactoring
configuration to be more generic, so there is some level of duplicated
code. I couldn't find an easy way to make the whole config process more
generic without much more re-writing.

Signed-off-by: Jacob Keller <***@intel.com>
---
makefile | 2 +-
phc2sys.8 | 174 +++++++++++++++++++++++-
phc2sys.c | 454 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 626 insertions(+), 4 deletions(-)

diff --git a/makefile b/makefile
index 5469301af259..2a4ad9b6de2e 100644
--- a/makefile
+++ b/makefile
@@ -51,7 +51,7 @@ pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \

phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o phc.o phc2sys.o pi.o \
pmc_common.o print.o raw.o servo.o sk.o stats.o sysoff.o tlv.o \
- transport.o udp.o udp6.o uds.o util.o version.o
+ transport.o udp.o udp6.o uds.o util.o version.o config.o

hwstamp_ctl: hwstamp_ctl.o version.o

diff --git a/phc2sys.8 b/phc2sys.8
index 22d02c24db7a..ae4faa8d45b8 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -3,6 +3,9 @@
phc2sys \- synchronize two or more clocks

.SH SYNOPSIS
+.B phc2sys \-f
+[ options ]
+.br
.B phc2sys \-a
[
.B \-r
@@ -33,7 +36,7 @@ program.

With the
.B \-a
-option, the clocks to synchronize are fetched from the running
+option (automatic mode), the clocks to synchronize are fetched from the running
.B ptp4l
daemon and the direction of synchronization automatically follows changes of
the PTP port states.
@@ -47,6 +50,9 @@ and driver implementation.

.SH OPTIONS
.TP
+.BI \-f
+Read configuration from the specified file. No configuration is read by default.
+.TP
.BI \-a
Read the clocks to synchronize from running
.B ptp4l
@@ -218,6 +224,172 @@ Display a help message.
.B \-v
Prints the software version and exits.

+.SH CONFIGURATION FILE
+
+The configuration file is divided into sections, and may be shared with ptp4l.
+Each section starts with a line containing its name enclosed in brackets and it
+follows with settings. Each setting is placed on a separate line, it contains
+the name of the option and the value separated by whitespace characters. Empty
+lines and lines starting with # are ignored.
+
+The global section (indicated as
+.BR [global] )
+sets generic program options mostly used by ptp4l, as well as some options
+shared by phc2sys. Note that only the options relevant to phc2sys are outlined
+here. See ptp4l(8) for options relevant to the ptp4l program.
+
+.SH GENERAL PROGAM OPTIONS
+
+.TP
+.B domainNumber
+The domain attribute of the local clock.
+The default is 0.
+.TP
+.B clock_servo
+The servo which is used to synchronize the local clock. Valid values are pi for
+a PI controller, linreg for an adaptive controller using linear regression, and
+ntpshm for the NTP SHM reference clock to allow another process to synchronize
+the local clock (the SHM segment number is set to the domain number).
+The default is pi.
+.TP
+.B pi_proportional_const
+The proportional constant of the PI controller. When set to 0.0, the
+proportional constant will be set by the following formula from the current
+sync interval.
+The default is 0.0.
+
+kp = min(kp_scale * sync^kp_exponent, kp_norm_max / sync))
+.TP
+.B pi_integral_const
+The integral constant of the PI controller. When set to 0.0, the
+integral constant will be set by the following formula from the current
+sync interval.
+The default is 0.0.
+
+ki = min(ki_scale * sync^ki_exponent, ki_norm_max / sync)
+.TP
+.B step_threshold
+The maximum offset the servo will correct by changing the clock
+frequency instead of stepping the clock. When set to 0.0, the servo will
+never step the clock except on start. It's specified in seconds.
+The default is 0.0.
+This option used to be called
+.BR pi_offset_const .
+.TP
+.B first_step_threshold
+The maximum offset the servo will correct by changing the clock
+frequency instead of stepping the clock. This is only applied on the first
+update. It's specified in seconds. When set to 0.0, the servo won't step
+the clock on start.
+The default is 0.00002 (20 microseconds).
+This option used to be called
+.BR pi_f_offset_const .
+.TP
+.B sanity_freq_limit
+The maximum allowed frequency offset between uncorrected clock and the system
+monotonic clock in parts per billion (ppb). This is used as a sanity check of
+the synchronized clock. When a larger offset is measured, a warning message
+will be printed and the servo will be reset. When set to 0, the sanity check is
+disabled. The default is 200000000 (20%).
+.TP
+.B ntpshm_segment
+The number of the SHM segment used by ntpshm servo.
+The default is 0.
+.TP
+.B uds_address
+Specifies the address of the UNIX domain socket for connecting to the local
+ptp4l(8) instance. The default is /var/run/ptp4l.
+.TP
+.B logging_level
+The maximum logging level of messages which should be printed.
+The default is 6 (LOG_INFO).
+.TP
+.B verbose
+Print messages to the standard output if enabled.
+The default is 0 (disabled).
+.TP
+.B use_syslog
+Print messages to the system log if enabled.
+The default is 1 (enabled).
+.TP
+.B kernel_leap
+When a leap second is announced, let the kernel apply it by stepping the clock
+instead of correcting the one-second offset with servo, which would correct the
+one-second offset slowly by changing the clock frequency (unless the
+.B step_threshold
+option is set to correct such offset by stepping).
+Relevant only with software time stamping. The default is 1 (enabled).
+
+.SH PHC2SYS SPECIFIC OPTIONS
+
+Note that the general options above may be re-specified in the
+.B [phc2sys]
+section. In this case, the value will override any setting on the
+.B [global]
+section. This is to allow use of a single configuration file for both phc2sys
+and ptp4l if desired.
+
+.TP
+.B autoconfig
+Enable automatic configuration mode. Default is 0 (disabled). See above section for more details.
+
+.TP
+.B realtime_mode
+Set to
+.BI ignore
+to disable use of CLOCK_REALTIME. Set to
+.BI sync
+to sync CLOCK_REALTIME system clock. Set to
+.BI follow
+to enable CLOCK_REALTIME as a possible time source. Only valid when in automatic configuration mode. Default is "ignore". See also
+.BI \-r
+option above for more details.
+
+.TP
+.B slave_clock
+Set clock device to use as slave in manual mode. No effect when in automatic mode.
+
+.TP
+.B master_clock
+Set clock device to use as master when in manual mode. No effect when in automatic mode.
+
+.TP
+.B master_pps
+Set the PPS device to use as the PPS signal for the master clock. Disabled by default.
+
+.TP
+.B wait_for_ptp4l
+Specify whether to wait for ptp4l to synchronize before beginning adjustment.
+Default is disabled.
+
+.TP
+.B update_rate
+Specify the slave clock update rate when running in the direct synchronization
+mode. Default is 1 per second.
+
+.TP
+.B phc_readings
+Specify the number of master clock readings per one slave clock update. Only
+the fastest reading is used to update the slave clock. This may minimize error
+caused by random delays in scheduling and bus utilization. Default is 5.
+
+.TP
+.B sync_offset
+Specify the offset between the slave and master time in seconds. Note this option is not compatible with automatic configuration. See
+.SM
+.B TIME SCALE USAGE
+below.
+
+.TP
+.BI summary_updates
+Specify the number of clock updates included in summary statistics. The
+statistics include offset root mean square (RMS), maximum absolute offset,
+frequency offset mean and standard deviation, and mean of the delay in clock
+readings and standard deviation. The units are nanoseconds and parts per
+billion (ppb). If zero, the individual samples are printed instead of the
+statistics. The messages are printed at the LOG_INFO level.
+The default is 0 (disabled).
+
.SH TIME SCALE USAGE

.B Ptp4l
diff --git a/phc2sys.c b/phc2sys.c
index d59814d3159f..029bfc1e38e4 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -19,6 +19,7 @@
*/
#include <errno.h>
#include <fcntl.h>
+#include <ctype.h>
#include <float.h>
#include <inttypes.h>
#include <limits.h>
@@ -56,6 +57,7 @@
#include "uds.h"
#include "util.h"
#include "version.h"
+#include "config.h"

#define KP 0.7
#define KI 0.3
@@ -1170,6 +1172,398 @@ static int clock_handle_leap(struct node *node, struct clock *clock,
return 0;
}

+#define CFG_IGNORE_AUTOCFG (1 << 7)
+#define CFG_IGNORE_REALTIME (1 << 8)
+#define CFG_IGNORE_SLAVE_CLOCK (1 << 9)
+#define CFG_IGNORE_MASTER_PPS (1 << 10)
+#define CFG_IGNORE_MASTER_CLOCK (1 << 11)
+#define CFG_IGNORE_CLOCK_SERVO (1 << 12)
+#define CFG_IGNORE_PI_KP (1 << 13)
+#define CFG_IGNORE_PI_KI (1 << 14)
+#define CFG_IGNORE_STEP_THRESHOLD (1 << 15)
+#define CFG_IGNORE_FIRST_STEP_THRESHOLD (1 << 16)
+#define CFG_IGNORE_PHC_RATE (1 << 17)
+#define CFG_IGNORE_PHC_READINGS (1 << 18)
+#define CFG_IGNORE_SYNC_OFFSET (1 << 19)
+#define CFG_IGNORE_FREQ_LIMIT (1 << 20)
+#define CFG_IGNORE_NTPSHM_SEGMENT (1 << 21)
+#define CFG_IGNORE_STATS_MAX_COUNT (1 << 22)
+#define CFG_IGNORE_WAIT_SYNC (1 << 23)
+#define CFG_IGNORE_DOMAIN_NUMBER (1 << 24)
+#define CFG_IGNORE_KERNEL_LEAP (1 << 25)
+#define CFG_IGNORE_UDS_ADDRESS (1 << 26)
+
+struct phc_config {
+ /* configuration override */
+ int cfg_ignore;
+
+ struct node *node;
+
+ /* automatic mode */
+ int *autoconfig;
+ int *realtime;
+
+ /* manual mode */
+ char **slave_clock;
+ char **master_clock;
+ int *master_pps_fd;
+ int *wait_for_ptp4l;
+
+ double *step_threshold;
+ double *first_step_threshold;
+
+ double *pi_proportional_const;
+ double *pi_integral_const;
+ int *ntpshm_segment;
+
+ int *domain_number;
+
+ char *uds_address;
+
+ int *print_level;
+ int *use_syslog;
+ int *verbose;
+};
+
+static enum parser_result parse_phc2sys_global_setting(const char *option,
+ const char *value,
+ struct phc_config *cfg,
+ int ignore)
+{
+ double df;
+ int val;
+ unsigned int uval;
+
+ enum parser_result r;
+
+ if (!strcmp(option, "domainNumber")) {
+ r = get_ranged_uint(value, &uval, 0, 127);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_DOMAIN_NUMBER))
+ *cfg->domain_number = uval;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_DOMAIN_NUMBER;
+
+ } else if (!strcmp(option, "pi_proportional_const")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_PI_KP))
+ *cfg->pi_proportional_const = df;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_PI_KP;
+
+ } else if (!strcmp(option, "pi_integral_const")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_PI_KI))
+ *cfg->pi_integral_const = df;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_PI_KI;
+
+ } else if (!strcmp(option, "step_threshold")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_STEP_THRESHOLD))
+ *cfg->step_threshold = df;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_STEP_THRESHOLD;
+
+ } else if (!strcmp(option, "first_step_threshold")) {
+ r = get_ranged_double(value, &df, 0.0, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_FIRST_STEP_THRESHOLD))
+ *cfg->first_step_threshold = df;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_FIRST_STEP_THRESHOLD;
+
+ } else if (!strcmp(option, "sanity_freq_limit")) {
+ r = get_ranged_int(value, &val, 0, INT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_FREQ_LIMIT))
+ cfg->node->sanity_freq_limit = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_FREQ_LIMIT;
+
+ } else if (!strcmp(option, "ntpshm_segment")) {
+ r = get_ranged_int(value, &val, INT_MIN, INT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_NTPSHM_SEGMENT))
+ *cfg->ntpshm_segment = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_NTPSHM_SEGMENT;
+
+ } else if (!strcmp(option, "uds_address")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_UDS_ADDRESS))
+ strncpy(cfg->uds_address, value, MAX_IFNAME_SIZE);
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_UDS_ADDRESS;
+
+ } else if (!strcmp(option, "logging_level")) {
+ r = get_ranged_int(value, &val,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_PRINT_LEVEL))
+ *cfg->print_level = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_PRINT_LEVEL;
+
+ } else if (!strcmp(option, "verbose")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_VERBOSE))
+ *cfg->verbose = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_VERBOSE;
+
+ } else if (!strcmp(option, "use_syslog")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_USE_SYSLOG))
+ *cfg->use_syslog = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_USE_SYSLOG;
+
+ } else if (!strcmp(option, "clock_servo")) {
+ if (!(cfg->cfg_ignore & CFG_IGNORE_CLOCK_SERVO)) {
+ if (!strcasecmp("pi", value))
+ cfg->node->servo_type = CLOCK_SERVO_PI;
+ else if (!strcasecmp("linreg", value))
+ cfg->node->servo_type = CLOCK_SERVO_LINREG;
+ else if (!strcasecmp("ntpshm", value))
+ cfg->node->servo_type = CLOCK_SERVO_NTPSHM;
+ else
+ return BAD_VALUE;
+ }
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_CLOCK_SERVO;
+
+ } else if (!strcmp(option, "kernel_leap")) {
+ r = get_ranged_int(value, &val, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_KERNEL_LEAP))
+ cfg->node->kernel_leap = val;
+ if (ignore)
+ cfg->cfg_ignore |= CFG_IGNORE_KERNEL_LEAP;
+
+ } else
+ return NOT_PARSED;
+
+ return PARSED_OK;
+}
+
+static enum parser_result parse_phc2sys_setting(const char *option,
+ const char *value,
+ struct phc_config *cfg)
+{
+ double df;
+ int i, val;
+ unsigned uval;
+
+ enum parser_result r;
+
+ r = parse_phc2sys_global_setting(option, value, cfg, 1);
+ if (r != NOT_PARSED)
+ return r;
+
+ if (!strcmp(option, "autoconfig")) {
+ r = get_ranged_uint(value, &uval, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_AUTOCFG))
+ *cfg->autoconfig = uval;
+
+ } else if (!strcmp(option, "realtime_mode")) {
+ if (!(cfg->cfg_ignore & CFG_IGNORE_REALTIME)) {
+ if (!strcasecmp("ignore", value))
+ *cfg->realtime = 0;
+ else if (!strcasecmp("sync", value))
+ *cfg->realtime = 1;
+ else if(!strcasecmp("follow", value))
+ *cfg->realtime = 2;
+ else
+ return BAD_VALUE;
+ }
+
+ } else if (!strcmp(option, "slave_clock")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_SLAVE_CLOCK))
+ strncpy(*cfg->slave_clock, value, MAX_IFNAME_SIZE);
+
+ } else if (!strcmp(option, "master_clock")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_MASTER_CLOCK))
+ strncpy(*cfg->master_clock, value, MAX_IFNAME_SIZE);
+
+ } else if (!strcmp(option, "master_pps")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_MASTER_PPS)) {
+ i = open(value, O_RDONLY);
+ if (i < 0) {
+ fprintf(stderr, "cannot open '%s': %m\n", value);
+ return BAD_VALUE;
+ }
+ *cfg->master_pps_fd = i;
+ }
+
+ } else if (!strcmp(option, "wait_for_ptp4l")) {
+ r = get_ranged_uint(value, &uval, 0, 1);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_WAIT_SYNC))
+ *cfg->wait_for_ptp4l = uval;
+
+ } else if (!strcmp(option, "update_rate")) {
+ r = get_ranged_double(value, &df, 1e-9, DBL_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_PHC_RATE))
+ cfg->node->phc_interval = 1.0 / df;
+
+ } else if (!strcmp(option, "phc_readings")) {
+ r = get_ranged_uint(value, &uval, 1, INT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_PHC_READINGS))
+ cfg->node->phc_readings = uval;
+
+ } else if (!strcmp(option, "sync_offset")) {
+ r = get_ranged_int(value, &val, INT_MIN, INT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_SYNC_OFFSET)) {
+ cfg->node->sync_offset = val;
+ cfg->node->forced_sync_offset = -1;
+ }
+
+ } else if (!strcmp(option, "summary_updates")) {
+ r = get_ranged_uint(value, &uval, 0, UINT_MAX);
+ if (r != PARSED_OK)
+ return r;
+ if (!(cfg->cfg_ignore & CFG_IGNORE_STATS_MAX_COUNT))
+ cfg->node->stats_max_count = uval;
+
+ } else
+ return NOT_PARSED;
+
+ return PARSED_OK;
+}
+
+static int phc_config_read(char *name, struct phc_config *cfg)
+{
+ enum config_section current_section = UNKNOWN_SECTION;
+ enum parser_result parser_res;
+ FILE *fp;
+ char buf[1024], *line, *c;
+ char *section_name;
+ const char *option, *value;
+ int line_num;
+
+ fp = 0 == strncmp(name, "-", 2) ? stdin : fopen(name, "r");
+
+ if (!fp) {
+ fprintf(stderr, "failed to open configuration file %s: %m\n", name);
+ return -1;
+ }
+
+ for (line_num = 1; fgets(buf, sizeof(buf), fp); line_num++) {
+ c = buf;
+
+ /* skip whitespace characters */
+ while(isspace(*c))
+ c++;
+
+ /* ignore empty lines and comments */
+ if (*c == '#' || *c == '\n' || *c == '\0')
+ continue;
+
+ line = c;
+
+ /* remote trailing whitespace characters and newlines */
+ c += strlen(line) - 1;
+ while (c > line && (*c == '\n' || isspace(*c)))
+ *c-- = '\0';
+
+ /* parse section lines */
+ if (parse_section_line(line, &current_section) == PARSED_OK)
+ continue;
+
+ if (parse_setting_line(line, &option, &value)) {
+ fprintf(stderr, "could not parse line %d\n",
+ line_num);
+ goto parse_error;
+ }
+
+ check_deprecated_options(&option);
+
+ switch (current_section) {
+ case GLOBAL_SECTION:
+ section_name = "global";
+ parser_res = parse_phc2sys_global_setting(option, value, cfg, 0);
+ if (parser_res == NOT_PARSED)
+ parser_res = parse_known_global_options(option);
+
+ break;
+ case PHC2SYS_SECTION:
+ section_name = "phy2sys";
+ parser_res = parse_phc2sys_setting(option, value, cfg);
+ break;
+ case UNKNOWN_SECTION:
+ fprintf(stderr, "line %d is not in a section\n", line_num);
+ goto parse_error;
+ /* ignore per-port settings*/
+ case PORT_SECTION:
+ default:
+ continue;
+ }
+
+ switch (parser_res) {
+ case PARSED_OK:
+ break;
+ case NOT_PARSED:
+ fprintf(stderr, "unknown option %s at line %d in %s section\n",
+ option, line_num,
+ section_name);
+ goto parse_error;
+ case BAD_VALUE:
+ fprintf(stderr, "%s is a bad value for option %s at line %d\n",
+ value, option, line_num);
+ goto parse_error;
+ case MALFORMED:
+ fprintf(stderr, "%s is a malformed value for option %s at line %d\n",
+ value, option, line_num);
+ goto parse_error;
+ case OUT_OF_RANGE:
+ fprintf(stderr, "%s is an out of range value for option %s at line %d\n",
+ value, option, line_num);
+ goto parse_error;
+ }
+ }
+
+ fclose(fp);
+ return 0;
+
+parse_error:
+ fprintf(stderr, "failed to parse configuration file %s\n", name);
+ fclose(fp);
+ return -2;
+}
+
static void usage(char *progname)
{
fprintf(stderr,
@@ -1187,6 +1581,7 @@ static void usage(char *progname)
" -O [offset] slave-master time offset (0)\n"
" -w wait for ptp4l\n"
" common options:\n"
+ " -f [file] read configuration from 'file'\n"
" -E [pi|linreg] clock servo (pi)\n"
" -P [kp] proportional constant (0.7)\n"
" -I [ki] integration constant (0.3)\n"
@@ -1211,7 +1606,7 @@ static void usage(char *progname)

int main(int argc, char *argv[])
{
- char *progname;
+ char *progname, *config;
char *src_name = NULL, *dst_name = NULL;
struct clock *src, *dst;
int autocfg = 0, rt = 0;
@@ -1227,6 +1622,31 @@ int main(int argc, char *argv[])
.kernel_leap = 1,
};

+ struct phc_config cfg_settings = {
+ .node = &node,
+ .autoconfig = &autocfg,
+ .realtime = &rt,
+
+ .wait_for_ptp4l = &wait_sync,
+ .slave_clock = &dst_name,
+ .master_clock = &src_name,
+ .master_pps_fd = &pps_fd,
+
+ .pi_proportional_const = &configured_pi_kp,
+ .pi_integral_const = &configured_pi_ki,
+ .step_threshold = &servo_step_threshold,
+ .first_step_threshold = &servo_first_step_threshold,
+
+ .ntpshm_segment = &ntpshm_segment,
+ .domain_number = &domain_number,
+
+ .uds_address = uds_path,
+
+ .print_level = &print_level,
+ .verbose = &verbose,
+ .use_syslog = &use_syslog,
+ };
+
handle_term_signals();

configured_pi_kp = KP;
@@ -1236,16 +1656,19 @@ int main(int argc, char *argv[])
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
while (EOF != (c = getopt(argc, argv,
- "arc:d:s:E:P:I:S:F:R:N:O:L:M:i:u:wn:xz:l:mqvh"))) {
+ "arc:d:s:f:E:P:I:S:F:R:N:O:L:M:i:u:wn:xz:l:mqvh"))) {
switch (c) {
case 'a':
autocfg = 1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_AUTOCFG;
break;
case 'r':
rt++;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_REALTIME;
break;
case 'c':
dst_name = strdup(optarg);
+ cfg_settings.cfg_ignore |= CFG_IGNORE_SLAVE_CLOCK;
break;
case 'd':
pps_fd = open(optarg, O_RDONLY);
@@ -1254,12 +1677,17 @@ int main(int argc, char *argv[])
"cannot open '%s': %m\n", optarg);
return -1;
}
+ cfg_settings.cfg_ignore |= CFG_IGNORE_MASTER_PPS;
break;
case 'i':
fprintf(stderr,
"'-i' has been deprecated. please use '-s' instead.\n");
case 's':
src_name = strdup(optarg);
+ cfg_settings.cfg_ignore |= CFG_IGNORE_MASTER_CLOCK;
+ break;
+ case 'f':
+ config = optarg;
break;
case 'E':
if (!strcasecmp(optarg, "pi")) {
@@ -1273,64 +1701,78 @@ int main(int argc, char *argv[])
"invalid servo name %s\n", optarg);
return -1;
}
+ cfg_settings.cfg_ignore |= CFG_IGNORE_CLOCK_SERVO;
break;
case 'P':
if (get_arg_val_d(c, optarg, &configured_pi_kp,
0.0, DBL_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_PI_KP;
break;
case 'I':
if (get_arg_val_d(c, optarg, &configured_pi_ki,
0.0, DBL_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_PI_KI;
break;
case 'S':
if (get_arg_val_d(c, optarg, &servo_step_threshold,
0.0, DBL_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_STEP_THRESHOLD;
break;
case 'F':
if (get_arg_val_d(c, optarg, &servo_first_step_threshold,
0.0, DBL_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_FIRST_STEP_THRESHOLD;
break;
case 'R':
if (get_arg_val_d(c, optarg, &phc_rate, 1e-9, DBL_MAX))
return -1;
node.phc_interval = 1.0 / phc_rate;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_PHC_RATE;
break;
case 'N':
if (get_arg_val_i(c, optarg, &node.phc_readings, 1, INT_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_PHC_READINGS;
break;
case 'O':
if (get_arg_val_i(c, optarg, &node.sync_offset,
INT_MIN, INT_MAX))
return -1;
node.forced_sync_offset = -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_SYNC_OFFSET;
break;
case 'L':
if (get_arg_val_i(c, optarg, &node.sanity_freq_limit, 0, INT_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_FREQ_LIMIT;
break;
case 'M':
if (get_arg_val_i(c, optarg, &ntpshm_segment, INT_MIN, INT_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_NTPSHM_SEGMENT;
break;
case 'u':
if (get_arg_val_ui(c, optarg, &node.stats_max_count,
0, UINT_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_STATS_MAX_COUNT;
break;
case 'w':
wait_sync = 1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_WAIT_SYNC;
break;
case 'n':
if (get_arg_val_i(c, optarg, &domain_number, 0, 255))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_DOMAIN_NUMBER;
break;
case 'x':
node.kernel_leap = 0;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_KERNEL_LEAP;
break;
case 'z':
if (strlen(optarg) > MAX_IFNAME_SIZE) {
@@ -1339,17 +1781,21 @@ int main(int argc, char *argv[])
return -1;
}
strncpy(uds_path, optarg, MAX_IFNAME_SIZE);
+ cfg_settings.cfg_ignore |= CFG_IGNORE_UDS_ADDRESS;
break;
case 'l':
if (get_arg_val_i(c, optarg, &print_level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX))
return -1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_PRINT_LEVEL;
break;
case 'm':
verbose = 1;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_VERBOSE;
break;
case 'q':
use_syslog = 0;
+ cfg_settings.cfg_ignore |= CFG_IGNORE_USE_SYSLOG;
break;
case 'v':
version_show(stdout);
@@ -1362,6 +1808,10 @@ int main(int argc, char *argv[])
}
}

+ if (config && (c = phc_config_read(config, &cfg_settings))) {
+ return c;
+ }
+
if (autocfg && (src_name || dst_name || pps_fd >= 0 || wait_sync || node.forced_sync_offset)) {
fprintf(stderr,
"autoconfiguration cannot be mixed with manual config options.\n");
--
2.1.2.555.gfbecd99
Keller, Jacob E
2015-01-12 22:03:42 UTC
Permalink
Jacob,
I am glad to see this patch series. With all of its command line
options, having a configuration file for phc2sys would be a usability
improvement.
Post by Jacob Keller
I didn't think the effort necessary to re-write the configuration such
that it was more generic was worth it. Thus, there is some level of code
duplication as phc2sys does not need the same configuration values as
ptp4l.
If you have the time, I would welcome an overhaul of the config code.
My feeling is that the config code is growing a bit ugly, and it
certainly is smarter to clean it up before trying to expand it.
I can look at this though it will take a lot more real time since I have
less to dedicate to it. But I am interested in solving this.
- Model the configuration file in memory explicitly.
- Each program (ptp4l, phc2sys, and pmc, too) reads the file early on,
and then queries the model for the parameters it needs.
- Get rid of all of the pointers in 'struct config'. They are a bit
hacky, what with points to random globals in the various modules.
- Instead, each configuration parameter could look like this.
struct config_item {
char label[N];
int flags; /* bit field of allowed sections */
/* maybe ignore bits says, "was set on command line" */
union {
int i;
double d;
unsigned char *s;
} value;
};
What about section settings for things like the per-adapter stuff? This
is where it got really complicated.

Other question I have regarding this, is should we share options between
programs? I think this is ok, because in reality if you *need* differing
options for each program you can just use a separate file.
- The modules which now have globals can query the configuration model
$ grep extern *.h
msg.h:extern int assume_two_step;
ntpshm.h:extern int ntpshm_segment;
pi.h:extern double configured_pi_kp;
pi.h:extern double configured_pi_ki;
pi.h:extern double configured_pi_kp_scale;
pi.h:extern double configured_pi_kp_exponent;
pi.h:extern double configured_pi_kp_norm_max;
pi.h:extern double configured_pi_ki_scale;
pi.h:extern double configured_pi_ki_exponent;
pi.h:extern double configured_pi_ki_norm_max;
raw.h:extern unsigned char ptp_dst_mac[];
raw.h:extern unsigned char p2p_dst_mac[];
servo.h:extern double servo_step_threshold;
servo.h:extern double servo_first_step_threshold;
servo.h:extern int servo_max_frequency;
sk.h:extern int sk_tx_timeout;
sk.h:extern int sk_check_fupsync;
tlv.h:extern uint8_t ieee8021_id[3];
udp6.h:extern unsigned char udp6_scope;
uds.h:extern char uds_path[MAX_IFNAME_SIZE + 1];
Post by Jacob Keller
The intention is to enable additional non-command line options for
phc2sys. We could even deprecate several current phc2sys options which
are no longer necessary (similar to ptp4l which has many config options
but few command line parameters).
Yes.
Post by Jacob Keller
In addition this opens the door to bring several settings from ptp4l
into phc2sys (like the PI servo settings).
Yes, yes.
Thanks,
Richard
Richard Cochran
2015-01-15 11:42:26 UTC
Permalink
Post by Keller, Jacob E
I can look at this though it will take a lot more real time since I have
less to dedicate to it. But I am interested in solving this.
If you are agreeable, then I would say let's take the extra time to do
it right. We just cut a release, and so there is no rush.
Post by Keller, Jacob E
What about section settings for things like the per-adapter stuff? This
is where it got really complicated.
Yup, also for per-port defaults, we need to put on our thinking caps,
but it shouldn't mean rocket science.
Post by Keller, Jacob E
Other question I have regarding this, is should we share options between
programs? I think this is ok, because in reality if you *need* differing
options for each program you can just use a separate file.
Agreed. Most or even all relevant options (eg domain number) can be
shared between ptp4l, phc2sys, and pmc. I can't think of any option
that can't be shared, but maybe we'll find one along the way.

Thanks,
Richard
Keller, Jacob E
2015-01-19 18:05:07 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
I can look at this though it will take a lot more real time since I have
less to dedicate to it. But I am interested in solving this.
If you are agreeable, then I would say let's take the extra time to do
it right. We just cut a release, and so there is no rush.
Makes sense. Partly why I waited to submit code since I didn't think
what I had was ready yet.
Post by Richard Cochran
Post by Keller, Jacob E
What about section settings for things like the per-adapter stuff? This
is where it got really complicated.
Yup, also for per-port defaults, we need to put on our thinking caps,
but it shouldn't mean rocket science.
Ya. I think I have a solution for this.
Post by Richard Cochran
Post by Keller, Jacob E
Other question I have regarding this, is should we share options between
programs? I think this is ok, because in reality if you *need* differing
options for each program you can just use a separate file.
Agreed. Most or even all relevant options (eg domain number) can be
shared between ptp4l, phc2sys, and pmc. I can't think of any option
that can't be shared, but maybe we'll find one along the way.
Well plenty of options are specific to ptp4l and don't really apply to
other programs. Some options that affect verbosity might be the ones
which people could want differing values for each program. In either
case I think the correct answer is "use separate config files if you
need separate values". This is the easiest and cleanest way.
Post by Richard Cochran
Thanks,
Richard
Thanks,
Jake
Richard Cochran
2015-01-19 19:15:41 UTC
Permalink
Post by Keller, Jacob E
Post by Richard Cochran
Post by Keller, Jacob E
What about section settings for things like the per-adapter stuff? This
is where it got really complicated.
Yup, also for per-port defaults, we need to put on our thinking caps,
but it shouldn't mean rocket science.
Ya. I think I have a solution for this.
Here is an idea that I have been thinking over (but not coding ;)

An config has a list for sections and a hash table for config items.

struct config {
list *sections;
struct hash *table;
};

A new config has 'sections' and 'table' initially empty. A 'section'
has a string name and nothing else (that I can think of now).

In addition to the other field I sketched before, the config_item also
has a pointer to the 'section' to which it belongs.

The items are hashed into the table using "section_name:item_name".

There is method to add a section, and there are methods to add, query,
and change items.

config_add_section(cfg, name);
config_add_item(cfg, section, name, default_val, range, type, ...);
config_get_item(cfg, section, name);
config_set_item(cfg, section, name, val); /* called by file/cmdline parser */

In addition, the file parser needs to know what to scan:

config_item_type(cfg, name); /* called by file parser */

The add/get/set methods can either work with a polymorphic union, or
they become a family of calls, with one flavor for each of int,
double, etc.

Now adding a new option just becomes adding two lines, one default
with ranges config_add_item() and one call to actually use the option
somewhere.

Thoughts?

Thanks,
Richard
Keller, Jacob E
2015-01-22 01:01:11 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
Post by Richard Cochran
Post by Keller, Jacob E
What about section settings for things like the per-adapter stuff? This
is where it got really complicated.
Yup, also for per-port defaults, we need to put on our thinking caps,
but it shouldn't mean rocket science.
Ya. I think I have a solution for this.
Here is an idea that I have been thinking over (but not coding ;)
An config has a list for sections and a hash table for config items.
struct config {
list *sections;
struct hash *table;
};
A new config has 'sections' and 'table' initially empty. A 'section'
has a string name and nothing else (that I can think of now).
In addition to the other field I sketched before, the config_item also
has a pointer to the 'section' to which it belongs.
The items are hashed into the table using "section_name:item_name".
There is method to add a section, and there are methods to add, query,
and change items.
config_add_section(cfg, name);
config_add_item(cfg, section, name, default_val, range, type, ...);
config_get_item(cfg, section, name);
config_set_item(cfg, section, name, val); /* called by file/cmdline parser */
config_item_type(cfg, name); /* called by file parser */
The add/get/set methods can either work with a polymorphic union, or
they become a family of calls, with one flavor for each of int,
double, etc.
Now adding a new option just becomes adding two lines, one default
with ranges config_add_item() and one call to actually use the option
somewhere.
Thoughts?
Thanks,
Richard
I like this approach. I'll probably avoid the polymorphic union being
passed around, as I dislike that it makes it too easy for client of
config code to mis-use the types. They should request the type they
want, and if the configuration option doesn't have that type it should
simply error out.

Then we can simply define the default values and available values in the
config block, and the file parser can simply set.

If nothing is set then it gets the standard default value :)

I really like this approach.

Hopefully I can get to actually coding it soon ;)

Is there a standard implementation of a HASH similar to STAIL_QUEUE?

Regards,
Jake
Richard Cochran
2015-01-22 07:06:20 UTC
Permalink
Post by Keller, Jacob E
Is there a standard implementation of a HASH similar to STAIL_QUEUE?
Not that I know of, but I do have a small, generic C implementation
sitting around somewhere. I will get it into shape and then post it.

Thanks,
Richard
Keller, Jacob E
2015-01-22 22:43:22 UTC
Permalink
Post by Richard Cochran
Post by Keller, Jacob E
Is there a standard implementation of a HASH similar to STAIL_QUEUE?
Not that I know of, but I do have a small, generic C implementation
sitting around somewhere. I will get it into shape and then post it.
Thanks,
Richard
what about using hsearch(3)? from #include <search.h>

Regards,
Jake
Richard Cochran
2015-01-23 07:28:33 UTC
Permalink
Post by Keller, Jacob E
what about using hsearch(3)? from #include <search.h>
Didn't know about that. Learn something new every day. Yes, let's
use it.

Thanks,
Richard

Loading...