Rework hardware probing and RTL-SDR helper startup

The lazy startup of the separate helper program for the RTL-SDR
backend on playback start was incorrect with respect to the expected
behavior the frequency setting verbs.  This was not visible during
usage by the radio application, but was triggering failures in
several tests in the pyagl binding wrapper test suite.  To facilitate
starting the helper during backend initialization, the probing part
of the backend "init" has been split into a separate "probe" function,
and all backends have been updated to reflect this change.  Logic has
been added to enforce that "init" is only called after "probe" has
succeeded for a backend, and a comment has been added to radio_impl.h
to document this intended behavior.

Bug-AGL: SPEC-3717

Signed-off-by: Scott Murray <scott.murray@konsulko.com>
Change-Id: Ic37331a92bae7cc01ee448e69894fa5f49d08a74

6 files changed, 192 insertions, 120 deletions

diff --git a/binding/radio-binding.c b/binding/radio-binding.c
index 3b8561d..8b5559c 100644
--- a/binding/radio-binding.c
+++ b/binding/radio-binding.c
@@ -678,36 +678,41 @@ static void onevent(afb_api_t api, const char *event, struct json_object *object
 
 static int init(afb_api_t api)
 {
-	// Look for RTL-SDR USB adapter
+	// Probe for radio backends
 	radio_impl_ops = &rtlsdr_impl_ops;
-	int rc = radio_impl_ops->init();
+	int rc = radio_impl_ops->probe();
 	if(rc != 0) {
 		// Look for Kingfisher Si4689
 		radio_impl_ops = &kf_impl_ops;
-		rc = radio_impl_ops->init();
+		rc = radio_impl_ops->probe();
 	}
 	if(rc != 0) {
 		radio_impl_ops = &tef665x_impl_ops;
-		rc = radio_impl_ops->init();
+		rc = radio_impl_ops->probe();
 	}
 	if (rc != 0) {
 		radio_impl_ops = &null_impl_ops;
-		rc = radio_impl_ops->init();
+		rc = radio_impl_ops->probe();
 	}
 	if (rc != 0) {
-		// We don't expect the null implementation to fail init, but just in case...
+		// We don't expect the null implementation to fail probe, but just in case...
 		AFB_API_ERROR(afbBindingV3root, "No radio device found, exiting");
+		return rc;
 	}
-	if(rc == 0) {
-		AFB_API_NOTICE(afbBindingV3root, "%s found\n", radio_impl_ops->name);
-		radio_impl_ops->set_frequency_callback(freq_callback, NULL);
-		if(radio_impl_ops->set_rds_callback)
-		{
-			radio_impl_ops->set_rds_callback(rds_callback);
-		}
-	} else {
+	// Try to initialize detected backend
+	rc = radio_impl_ops->init();
+	if(rc < 0) {
+		AFB_API_ERROR(afbBindingV3root,
+			      "%s initialization failed\n",
+			      radio_impl_ops->name);
 		return rc;
 	}
+	AFB_API_NOTICE(afbBindingV3root, "%s found\n", radio_impl_ops->name);
+	radio_impl_ops->set_frequency_callback(freq_callback, NULL);
+	radio_impl_ops->set_frequency_callback(freq_callback, NULL);
+	if(radio_impl_ops->set_rds_callback) {
+		radio_impl_ops->set_rds_callback(rds_callback);
+	}
 
 	rc = afb_daemon_require_api("signal-composer", 1);
 	if (rc) {
diff --git a/binding/radio_impl.h b/binding/radio_impl.h
index 8f1ee94..3e549ac 100644
--- a/binding/radio_impl.h
+++ b/binding/radio_impl.h
@@ -71,6 +71,9 @@ typedef struct
 typedef struct {
 	char *name;
 
+	int (*probe)(void);
+
+	/* NOTE: init should return -1 if called before probe has been called and returned success */
 	int (*init)(void);
 
 	void (*set_output)(const char *output);
diff --git a/binding/radio_impl_kingfisher.c b/binding/radio_impl_kingfisher.c
index dc59916..b7d243b 100644
--- a/binding/radio_impl_kingfisher.c
+++ b/binding/radio_impl_kingfisher.c
@@ -57,12 +57,13 @@ static fm_band_plan_t known_fm_band_plans[5] = {
 };
 
 static unsigned int bandplan = 0;
-static bool corking = false;
-static bool present = false;
+static bool corking;
+static bool present;
+static bool initialized;
 static uint32_t current_frequency;
 static int scan_valid_snr_threshold = 128;
 static int scan_valid_rssi_threshold = 128;
-static bool scanning = false;
+static bool scanning;
 
 // stream state
 static GstElement *pipeline;
@@ -104,16 +105,9 @@ static void *gstreamer_loop_thread(void *ptr)
 	return NULL;
 }
 
-static int kf_init(void)
+static int kf_probe(void)
 {
-	GKeyFile* conf_file;
-	int conf_file_present = 0;
 	struct stat statbuf;
-	char *value_str;
-	char cmd[SI_CTL_CMDLINE_MAXLEN];
-	int rc;
-	char gst_pipeline_str[GST_PIPELINE_LEN];
-	pthread_t thread_id;
 
 	if(present)
 		return 0;
@@ -126,6 +120,26 @@ static int kf_init(void)
 	if(stat(SI_CTL, &statbuf) != 0)
 		return -1;
 
+	present = true;
+	return 0;
+}
+
+static int kf_init(void)
+{
+	GKeyFile* conf_file;
+	bool conf_file_present = false;
+	char *value_str;
+	char cmd[SI_CTL_CMDLINE_MAXLEN];
+	int rc;
+	char gst_pipeline_str[GST_PIPELINE_LEN];
+	pthread_t thread_id;
+
+	if(!present)
+		return -1;
+
+	if(initialized)
+		return 0;
+
 	// Load settings from configuration file if it exists
 	conf_file = g_key_file_new();
 	if(conf_file &&
@@ -135,7 +149,7 @@ static int kf_init(void)
 				     NULL,
 				     G_KEY_FILE_KEEP_COMMENTS,
 				     NULL) == TRUE) {
-		conf_file_present = 1;
+		conf_file_present = true;
 
 		// Set band plan if it is specified
 		value_str = g_key_file_get_string(conf_file,
@@ -229,7 +243,7 @@ static int kf_init(void)
 	if(rc != 0)
 		return rc;
 
-	present = true;
+	initialized = true;
 	return 0;
 }
 
@@ -247,7 +261,7 @@ static void kf_set_frequency(uint32_t frequency)
 	char cmd[SI_CTL_CMDLINE_MAXLEN];
 	int rc;
 
-	if(!present)
+	if(!initialized)
 		return;
 
 	if(scanning)
@@ -363,7 +377,7 @@ static bool kf_get_corking_state(void)
 
 static void kf_start(void)
 {
-	if(!present)
+	if(!initialized)
 		return;
 
 	if(!running || corking) {
@@ -378,7 +392,7 @@ static void kf_stop(void)
 {
 	GstEvent *event;
 
-	if(present && running) {
+	if(initialized && running) {
 		// Stop pipeline
 		running = false;
 
@@ -416,7 +430,7 @@ static void kf_scan_start(radio_scan_direction_t direction,
 	uint32_t new_frequency = 0;
 	FILE *fp;
 
-	if(!present)
+	if(!initialized)
 		return;
 
 	if(scanning)
@@ -487,6 +501,7 @@ static void kf_set_stereo_mode(radio_stereo_mode_t mode)
 
 radio_impl_ops_t kf_impl_ops = {
 	.name = "Kingfisher Si4689",
+	.probe = kf_probe,
 	.init = kf_init,
 	.set_output = kf_set_output,
 	.get_frequency = kf_get_frequency,
diff --git a/binding/radio_impl_null.c b/binding/radio_impl_null.c
index a90835b..b66c025 100644
--- a/binding/radio_impl_null.c
+++ b/binding/radio_impl_null.c
@@ -52,6 +52,7 @@ static fm_band_plan_t known_fm_band_plans[5] = {
 
 static unsigned int bandplan;
 static bool present;
+static bool initialized;
 static bool active;
 static bool scanning;
 static uint32_t current_frequency;
@@ -60,7 +61,12 @@ static void *freq_callback_data;
 
 static uint32_t null_get_min_frequency(radio_band_t band);
 static void null_set_frequency(uint32_t frequency);
-//static void null_scan_stop(void);
+
+static int null_probe(void)
+{
+	present = true;
+	return 0;
+}
 
 static int null_init(void)
 {
@@ -69,7 +75,10 @@ static int null_init(void)
 	char *rootdir;
 	char *helper_path;
 
-	if(present)
+	if(!present)
+		return -1;
+
+	if(initialized)
 		return 0;
 
 	// Load settings from configuration file if it exists
@@ -103,7 +112,7 @@ static int null_init(void)
 	// Start off with minimum bandplan frequency
 	current_frequency = null_get_min_frequency(BAND_FM);
 
-	present = true;
+	initialized = true;
 	null_set_frequency(current_frequency);
 
 	return 0;
@@ -184,7 +193,7 @@ static uint32_t null_get_frequency_step(radio_band_t band)
 
 static void null_start(void)
 {
-	if(!present)
+	if(!initialized)
 		return;
 
 	if(active)
@@ -195,7 +204,7 @@ static void null_start(void)
 
 static void null_stop(void)
 {
-	if(!present)
+	if(!initialized)
 		return;
 
 	if (!active)
@@ -251,6 +260,7 @@ static void null_set_stereo_mode(radio_stereo_mode_t mode)
 
 radio_impl_ops_t null_impl_ops = {
 	.name = "null/mock radio",
+	.probe = null_probe,
 	.init = null_init,
 	.set_output = null_set_output,
 	.get_frequency = null_get_frequency,
diff --git a/binding/radio_impl_rtlsdr.c b/binding/radio_impl_rtlsdr.c
index 62ec623..2087d10 100644
--- a/binding/radio_impl_rtlsdr.c
+++ b/binding/radio_impl_rtlsdr.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017,2018 Konsulko Group
+ * Copyright (C) 2017,2018,2020 Konsulko Group
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -57,6 +57,7 @@ static int helper_in;
 static int helper_out;
 static pthread_mutex_t helper_mutex = PTHREAD_MUTEX_INITIALIZER;
 static bool present;
+static bool initialized;
 static bool active;
 static bool scanning;
 static uint32_t current_frequency;
@@ -128,16 +129,89 @@ static pid_t popen2(char *command, int *in_fd, int *out_fd)
 	return pid;
 }
 
-static int rtlsdr_init(void)
+static int rtlsdr_probe(void)
 {
-	GKeyFile *conf_file;
-	char *value_str;
 	char *rootdir;
 	char *helper_path;
 
 	if(present)
 		return 0;
 
+	rootdir = getenv("AFM_APP_INSTALL_DIR");
+	if(!rootdir)
+		return -1;
+
+	// Run helper to detect adapter
+	helper_path = malloc(HELPER_MAX);
+	if(!helper_path)
+		return -ENOMEM;
+	if(snprintf(helper_path, HELPER_MAX, "%s/bin/%s --detect", rootdir, HELPER_NAME) == HELPER_MAX) {
+		AFB_API_ERROR(afbBindingV3root, "Could not create command for \"%s --detect\"", HELPER_NAME);
+		return -EINVAL;
+	}
+	if(system(helper_path) != 0) {
+		free(helper_path);
+		return -1;
+	}
+
+	present = true;
+	return 0;
+}
+
+static int rtlsdr_start_helper(void)
+{
+	char *rootdir;
+	char *helper_path;
+	static bool helper_started = false;
+
+	if(!present || initialized)
+		return -1;
+
+	if(helper_started)
+		return 0;
+
+	rootdir = getenv("AFM_APP_INSTALL_DIR");
+	if(!rootdir)
+		return -1;
+
+	if(helper_output) {
+		// Indicate desired output to helper
+		AFB_API_INFO(afbBindingV3root, "Setting RADIO_OUTPUT=%s", helper_output);
+		setenv("RADIO_OUTPUT", helper_output, 1);
+	}
+
+	// Run helper
+	helper_path = malloc(HELPER_MAX);
+	if(!helper_path)
+		return -ENOMEM;
+	if(snprintf(helper_path, PATH_MAX, "%s/bin/%s", rootdir, HELPER_NAME) == PATH_MAX) {
+		AFB_API_ERROR(afbBindingV3root, "Could not create path to %s", HELPER_NAME);
+		return -EINVAL;
+	}
+	helper_pid = popen2(helper_path, &helper_out, &helper_in);
+	if(helper_pid < 0) {
+		AFB_API_ERROR(afbBindingV3root, "Could not run %s!", HELPER_NAME);
+		return -1;
+	}
+	AFB_API_DEBUG(afbBindingV3root, "%s started", HELPER_NAME);
+	helper_started = true;
+	free(helper_path);
+
+	return 0;
+}
+
+static int rtlsdr_init(void)
+{
+	GKeyFile *conf_file;
+	char *value_str;
+	int rc;
+
+	if(!present)
+		return -1;
+
+	if(initialized)
+		return 0;
+
 	// Load settings from configuration file if it exists
 	conf_file = g_key_file_new();
 	if(conf_file &&
@@ -168,25 +242,12 @@ static int rtlsdr_init(void)
 
 	// Start off with minimum bandplan frequency
 	current_frequency = rtlsdr_get_min_frequency(BAND_FM);
-
-	rootdir = getenv("AFM_APP_INSTALL_DIR");
-	if(!rootdir)
-		return -1;
-
-	// Run helper to detect adapter
-	helper_path = malloc(HELPER_MAX);
-	if(!helper_path)
-		return -ENOMEM;
-	if(snprintf(helper_path, HELPER_MAX, "%s/bin/%s --detect", rootdir, HELPER_NAME) == HELPER_MAX) {
-		AFB_API_ERROR(afbBindingV3root, "Could not create command for \"%s --detect\"", HELPER_NAME);
-		return -EINVAL;
-	}
-	if(system(helper_path) != 0) {
-		free(helper_path);
-		return -1;
+	rc = rtlsdr_start_helper();
+	if(rc != 0) {
+		return rc;
 	}
 
-	present = true;
+	initialized = true;
 	rtlsdr_set_frequency(current_frequency);
 
 	return 0;
@@ -212,7 +273,7 @@ static void rtlsdr_set_frequency(uint32_t frequency)
 	ssize_t rc;
 	uint32_t n;
 
-	if(!present)
+	if(!initialized)
 		return;
 
 	if(frequency < known_fm_band_plans[bandplan].min ||
@@ -298,62 +359,17 @@ static uint32_t rtlsdr_get_frequency_step(radio_band_t band)
 	return ret;
 }
 
-static int rtlsdr_start_helper(void)
-{
-	char *rootdir;
-	char *helper_path;
-	static bool helper_started = false;
-
-	if(!present)
-		return -1;
-
-	if(helper_started)
-		return 0;
-
-	rootdir = getenv("AFM_APP_INSTALL_DIR");
-	if(!rootdir)
-		return -1;
-
-	if(helper_output) {
-		// Indicate desired output to helper
-		AFB_API_INFO(afbBindingV3root, "Setting RADIO_OUTPUT=%s", helper_output);
-		setenv("RADIO_OUTPUT", helper_output, 1);
-	}
-
-	// Run helper
-	helper_path = malloc(HELPER_MAX);
-	if(!helper_path)
-		return -ENOMEM;
-	if(snprintf(helper_path, PATH_MAX, "%s/bin/%s", rootdir, HELPER_NAME) == PATH_MAX) {
-		AFB_API_ERROR(afbBindingV3root, "Could not create path to %s", HELPER_NAME);
-		return -EINVAL;
-	}
-	helper_pid = popen2(helper_path, &helper_out, &helper_in);
-	if(helper_pid < 0) {
-		AFB_API_ERROR(afbBindingV3root, "Could not run %s!", HELPER_NAME);
-		return -1;
-	}
-	AFB_API_DEBUG(afbBindingV3root, "%s started", HELPER_NAME);
-	helper_started = true;
-	free(helper_path);
-
-	return 0;
-}
-
 static void rtlsdr_start(void)
 {
-	if(!present)
-		return;
+	ssize_t rc;
+	char cmd[HELPER_CMD_MAXLEN];
 
-	if(active)
+	if(!initialized)
 		return;
 
-	if(rtlsdr_start_helper() < 0)
+	if(active)
 		return;
 
-	ssize_t rc;
-	char cmd[HELPER_CMD_MAXLEN];
-
 	snprintf(cmd, sizeof(cmd), "START\n");
 	pthread_mutex_lock(&helper_mutex);
 	rc = write(helper_in, cmd, strlen(cmd));
@@ -368,7 +384,7 @@ static void rtlsdr_start(void)
 static void rtlsdr_stop(void)
 {
 	ssize_t rc;
-	if(!present)
+	if(!initialized)
 		return;
 
 	if (!active)
@@ -470,6 +486,7 @@ static void rtlsdr_set_stereo_mode(radio_stereo_mode_t mode)
 
 radio_impl_ops_t rtlsdr_impl_ops = {
 	.name = "RTL-SDR USB adapter",
+	.probe = rtlsdr_probe,
 	.init = rtlsdr_init,
 	.set_output = rtlsdr_set_output,
 	.get_frequency = rtlsdr_get_frequency,
diff --git a/binding/radio_impl_tef665x.c b/binding/radio_impl_tef665x.c
index 504f155..9bad684 100644
--- a/binding/radio_impl_tef665x.c
+++ b/binding/radio_impl_tef665x.c
@@ -137,9 +137,10 @@ static band_plan_t known_am_band_plans[1] = {
 
 static unsigned int fm_bandplan = 2;
 static unsigned int am_bandplan = 0;
-static bool corking = false;
-static bool present = false;
-static bool scanning = false;
+static bool corking;
+static bool present;
+static bool initialized;
+static bool scanning;
 
 // stream state
 static GstElement *pipeline;
@@ -1626,7 +1627,8 @@ static int i2c_init(const char *i2c, int state, uint32_t *i2c_file_desc)
 static void tef665x_start(void)
 {
 	int ret;
-	 if(!present)
+
+	if(!initialized)
 		return;
 
 	_debug("file_desc ", file_desc);
@@ -2078,7 +2080,7 @@ static void tef665x_stop(void)
 	GstEvent *event;
 	audio_set_mute(file_desc, 1);
 
-	 if(present && running) {
+	 if(initialized && running) {
 		// Stop pipeline
 		running = false;
 		ret = gst_element_set_state(pipeline, GST_STATE_PAUSED);
@@ -2093,13 +2095,12 @@ static void tef665x_stop(void)
 	}
 }
 
-static int tef665x_init()
+static int tef665x_probe()
 {
-	char gst_pipeline_str[GST_PIPELINE_LEN];
 	int rc;
 
-	current_am_frequency = known_am_band_plans[am_bandplan].min;
-	current_fm_frequency = known_fm_band_plans[fm_bandplan].min;
+	if(present)
+		return 0;
 
 	rc = i2c_init(I2C_DEV, _open, &file_desc);
 	if(rc < 0) {
@@ -2114,6 +2115,24 @@ static int tef665x_init()
 		return -1;
 	}
 
+	present = true;
+	return 0;
+}
+
+static int tef665x_init()
+{
+	char gst_pipeline_str[GST_PIPELINE_LEN];
+	int rc;
+
+	if(!present)
+		return -1;
+
+	if(initialized)
+		return 0;
+
+	current_am_frequency = known_am_band_plans[am_bandplan].min;
+	current_fm_frequency = known_fm_band_plans[fm_bandplan].min;
+
 	current_band = BAND_AM;
 
 	radio_powerSwitch(file_desc, 1);
@@ -2150,12 +2169,12 @@ static int tef665x_init()
 	rc = gst_bus_add_watch(gst_element_get_bus(pipeline), (GstBusFunc) handle_message, NULL);
 	_debug("gst_bus_add_watch   rc", rc);
 
-	present = true;
-
 	//Initialize Mutex Lock for Scan and RDS
 	pthread_mutex_init(&scan_mutex, NULL);
 	pthread_mutex_init (&RDS_Mutex, NULL);
 
+	initialized = true;
+
 	tef665x_start();
 	return 0;
 }
@@ -2226,7 +2245,8 @@ static void tef665x_set_alternative_frequency(uint32_t frequency)
 static void tef665x_set_frequency(uint32_t frequency)
 {
 	uint32_t fd = 0;
-	if(!present)
+
+	if(!initialized)
 		return;
 
 	if(scanning)
@@ -2357,8 +2377,10 @@ static uint32_t tef665x_get_frequency_step(radio_band_t band)
 	}
 	return ret;
 }
+
 radio_impl_ops_t tef665x_impl_ops = {
 	.name = "TEF665x",
+	.probe = tef665x_probe,
 	.init = tef665x_init,
 	.start = tef665x_start,
 	.stop = tef665x_stop,