Completely overhaul the code

This gets rid of a lot of nasty callback stuff, and compartmentalises
the NTP code much better.

4 files changed, 129 insertions, 191 deletions

diff --git a/README.md b/README.md
index 778c661..2f117a1 100644
--- a/README.md
+++ b/README.md
@@ -44,9 +44,12 @@ brightness).  There is a ground pad conveniently placed between the two pins.
 
 (Hopefully your soldering is a little neater than mine).
 
+Optionally solder a small button between GPIO 16 and GND.  This will be a test
+button for checking the device is working properly (not illustrated above).
+
 You can use other GPIOs if you prefer - just change the definitions of
-`LED_RED` and `LED_GREEN` in `morningtown.c`.  The positions of 21 and 22 just
-happen to work well for the geometry of my own nightstand.
+`LED_RED`, `LED_GREEN` and `TEST_BUTTON` in `morningtown.c`.  The positions of
+16, 21 and 22 just happen to work well for the geometry of my own nightstand.
 
 
 Software
@@ -67,17 +70,20 @@ To set different wake-up times, edit routine `check_clock()` in
 Run `compile`, then copy `build/morningtown.uf2` to the Pico.
 
 
-Boot up sequence
-----------------
+Operation
+---------
 
-Connect the Pico to any USB power supply.  Afterwards:
+Connect the Pico to any USB power supply.  All three LEDs (red, green and the
+one on Pi board itself) will light briefly.  The device will then attempt to
+connect to the WLAN and synchronise its clock.  During this phase, the red LED
+will be lit continuously and the board LED will light as long as the WLAN is
+connected.  Once an NTP reply is received, all the LEDs will go dark until
+morning.
 
-1. All three LEDs (red, green and the one on Pi board itself) light for 2
-   seconds, then switch off.
-2. Board LED lights to indicate WLAN connection
-3. Flashing red LED indicates a problem.  It will try again shortly.
-4. Alternating red and green LEDs indicate successful NTP synchronisation.
-5. All LEDs turn off.  Everything dark until morning.
+When the (optional) test button is pressed (and held), the board LED will
+indicate a current WLAN connection, the green LED will indicate that NTP is
+synchronised, and the red LED (hopefully not lit) indicates some kind of error
+condition.
 
 
 Licence
diff --git a/morningtown.c b/morningtown.c
index 42b7746..5fd6d3f 100644
--- a/morningtown.c
+++ b/morningtown.c
@@ -33,91 +33,21 @@
 #define LED_RED 22
 #define TEST_BUTTON 16
 
-int ntp_sent = 0;
-int ntp_ok = 0;
 
-
-static void flash_red(int n)
-{
-	int i;
-
-	gpio_put(LED_GREEN, 0);
-	for ( i=0; i<10; i++ ) {
-		gpio_put(LED_RED, 1);
-		sleep_ms(100);
-		gpio_put(LED_RED, 0);
-		sleep_ms(100);
-	}
-}
-
-
-static void celebrate()
-{
-	int i;
-
-	for ( i=0; i<10; i++ ) {
-		gpio_put(LED_RED, 1);
-		gpio_put(LED_GREEN, 0);
-		sleep_ms(300);
-		gpio_put(LED_RED, 0);
-		gpio_put(LED_GREEN, 1);
-		sleep_ms(300);
-	}
-
-	gpio_put(LED_GREEN, 0);
-	gpio_put(LED_RED, 0);
-}
-
-
-static void check_clock()
+static void check_clock(int *pre_wake, int *wake_now)
 {
 	datetime_t t = {0};
 	rtc_get_datetime(&t);
 
 	if ( (t.hour == 7) && (t.min >= 15) ) {
-		gpio_put(LED_RED, 0);
-		gpio_put(LED_GREEN, 1);
+		*pre_wake = 1;
+		*wake_now = 0;
 	} else if ( (t.hour >= 8) && (t.hour < 12) ) {
-		gpio_put(LED_RED, 1);
-		gpio_put(LED_GREEN, 1);
+		*pre_wake = 0;
+		*wake_now = 1;
 	} else {
-		gpio_put(LED_RED, 0);
-		gpio_put(LED_GREEN, 0);
-	}
-}
-
-
-static void ntp_callback(int status)
-{
-	switch ( status ) {
-
-		case NTP_DNS_ERROR:
-		flash_red(5);
-		ntp_sent = 0;
-		break;
-
-		case NTP_DNS_NO_ADDR:
-		flash_red(10);
-		ntp_sent = 0;
-		break;
-
-		case NTP_BAD_REPLY:
-		flash_red(15);
-		ntp_sent = 0;
-		break;
-
-		case NTP_TIMEOUT:
-		flash_red(20);
-		ntp_sent = 0;
-		break;
-
-		case NTP_REQUEST_SENT:
-		break;
-
-		case NTP_REPLY_RECEIVED:
-		celebrate();
-		ntp_ok = 1;
-		break;
+		*pre_wake = 0;
+		*wake_now = 0;
 	}
 }
 
@@ -126,6 +56,8 @@ int main()
 {
 	NTP_T *ntp_state;
 	int last_conn;
+	int pre_wake = 0;
+	int wake_now = 0;
 
 	gpio_init(LED_GREEN);
 	gpio_init(LED_RED);
@@ -151,15 +83,13 @@ int main()
 
 	rtc_init();
 
-	ntp_state = ntp_init(ntp_callback);
+	ntp_state = ntp_init();
 	last_conn = 200;
 	while (1) {
 
 		watchdog_update();
 
 		int st = cyw43_wifi_link_status(&cyw43_state, CYW43_ITF_STA);
-		cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN,
-		                    !ntp_ok && (st == CYW43_LINK_JOIN));
 
 		if ( (st != CYW43_LINK_JOIN) && (last_conn > 100) ) {
 			cyw43_arch_wifi_connect_async(WIFI_SSID,
@@ -169,18 +99,30 @@ int main()
 			last_conn = 0;
 		}
 
-		if ( (st == CYW43_LINK_JOIN) && !ntp_sent ) {
-			ntp_send_request(ntp_state);
-			debug_print("sending NTP request\n");
-			ntp_sent = 1;
-		}
-
-		if ( ntp_ok ) {
-			check_clock();
+		if ( ntp_ok(ntp_state) ) {
+			check_clock(&pre_wake, &wake_now);
 		}
 
-		if ( ntp_ok && !gpio_get(TEST_BUTTON) ) {
-			gpio_put(LED_GREEN, 1);
+		/* Determine the LED status */
+		if ( gpio_get(TEST_BUTTON) == 0 ) {
+			/* Button pressed */
+			gpio_put(LED_GREEN, ntp_ok(ntp_state));
+			gpio_put(LED_RED, ntp_err(ntp_state));
+			cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN,
+			                    (st == CYW43_LINK_JOIN));
+		} else {
+			if ( !ntp_ok(ntp_state) ) {
+				/* Booting up */
+				cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN,
+				                    (st == CYW43_LINK_JOIN));
+				gpio_put(LED_RED, 1);
+				gpio_put(LED_GREEN, 0);
+			} else {
+				/* Normal operation */
+				gpio_put(LED_GREEN, pre_wake || wake_now);
+				gpio_put(LED_RED, wake_now);
+				cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN, 0);
+			}
 		}
 
 		last_conn += 1;
diff --git a/ntp_client.c b/ntp_client.c
index 38275c9..06f7aac 100644
--- a/ntp_client.c
+++ b/ntp_client.c
@@ -41,10 +41,11 @@
 
 typedef struct NTP_T_ {
 	ip_addr_t ntp_server_address;
-	bool dns_request_sent;
 	struct udp_pcb *ntp_pcb;
-	alarm_id_t ntp_resend_alarm;
-	void (*callback)(int status);
+	alarm_id_t send_alarm;
+	int err;
+	int ok;
+	int first;
 } NTP_T;
 
 
@@ -52,7 +53,7 @@ typedef struct NTP_T_ {
 #define NTP_MSG_LEN 48
 #define NTP_PORT 123
 #define NTP_DELTA 2208988800 // seconds between 1 Jan 1900 and 1 Jan 1970
-#define NTP_RESEND_TIME (10 * 1000)
+#define NTP_RESEND_TIME (5 * 1000 * 1000)
 
 
 static void set_rtc(time_t iutc)
@@ -76,29 +77,41 @@ static void set_rtc(time_t iutc)
 }
 
 
-static void ntp_result(NTP_T *state, int status, time_t *result)
+static void ntp_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p,
+                     const ip_addr_t *addr, u16_t port)
 {
-	debug_print("NTP result:\n");
-	if (status == NTP_REPLY_RECEIVED && result) {
-
-		debug_print("yay!\n");
-
-		if ( state->ntp_resend_alarm > 0 ) {
-			cancel_alarm(state->ntp_resend_alarm);
-			state->ntp_resend_alarm = 0;
-		}
+	NTP_T *state = (NTP_T*)arg;
+	uint8_t mode = pbuf_get_at(p, 0) & 0x7;
+	uint8_t stratum = pbuf_get_at(p, 1);
 
-		set_rtc(*result);
+	debug_print("got UDP: %i %i\n", addr, state->ntp_server_address);
+	debug_print("port %i len %i mode %i stratum %i\n", port, p->tot_len, mode, stratum);
 
+	if (ip_addr_cmp(addr, &state->ntp_server_address)
+	 && port == NTP_PORT
+	 && p->tot_len == NTP_MSG_LEN
+	 && mode == 0x4 && stratum != 0)
+	{
+		uint8_t seconds_buf[4] = {0};
+		pbuf_copy_partial(p, seconds_buf, sizeof(seconds_buf), 40);
+		uint32_t seconds_since_1900 = seconds_buf[0] << 24
+		                            | seconds_buf[1] << 16
+		                            | seconds_buf[2] << 8
+		                            | seconds_buf[3];
+		uint32_t seconds_since_1970 = seconds_since_1900 - NTP_DELTA;
+		time_t epoch = seconds_since_1970;
+		debug_print("second since 1970 = %i\n", epoch);
+		set_rtc(epoch);
+		state->err = 0;
+		state->ok = 1;
 	} else {
-		debug_print("other reply.\n");
+		state->err = 1;
 	}
-
-	state->callback(status);
-	state->dns_request_sent = false;
+	pbuf_free(p);
 }
 
 
+/* The real address must be in state->ntp_server_address by this point */
 static void ntp_request(NTP_T *state)
 {
 	debug_print("sending NTP UDP\n");
@@ -110,7 +123,7 @@ static void ntp_request(NTP_T *state)
 	udp_sendto(state->ntp_pcb, p, &state->ntp_server_address, NTP_PORT);
 	pbuf_free(p);
 	cyw43_arch_lwip_end();
-	state->callback(NTP_REQUEST_SENT);
+	state->err = 0;
 }
 
 
@@ -122,94 +135,80 @@ static void ntp_dns_found(const char *hostname,
 	if (ipaddr) {
 		debug_print("DNS ok (%i)\n", *ipaddr);
 		state->ntp_server_address = *ipaddr;
+		state->err = 0;
 		ntp_request(state);
 	} else {
 		debug_print("NTP failed: DNS no address\n");
-		ntp_result(state, NTP_DNS_NO_ADDR, NULL);
+		state->err = 1;
 	}
 }
 
 
-static void ntp_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p,
-                     const ip_addr_t *addr, u16_t port)
+
+static int64_t send_handler(alarm_id_t id, void *user_data)
 {
-	NTP_T *state = (NTP_T*)arg;
-	uint8_t mode = pbuf_get_at(p, 0) & 0x7;
-	uint8_t stratum = pbuf_get_at(p, 1);
+	int err;
+	NTP_T *state = (NTP_T*)user_data;
 
-	debug_print("got UDP: %i %i\n", addr, state->ntp_server_address);
-	debug_print("port %i len %i mode %i stratum %i\n", port, p->tot_len, mode, stratum);
-
-	if (ip_addr_cmp(addr, &state->ntp_server_address)
-	 && port == NTP_PORT
-	 && p->tot_len == NTP_MSG_LEN
-	 && mode == 0x4 && stratum != 0)
-	{
-		uint8_t seconds_buf[4] = {0};
-		pbuf_copy_partial(p, seconds_buf, sizeof(seconds_buf), 40);
-		uint32_t seconds_since_1900 = seconds_buf[0] << 24
-		                            | seconds_buf[1] << 16
-		                            | seconds_buf[2] << 8
-		                            | seconds_buf[3];
-		uint32_t seconds_since_1970 = seconds_since_1900 - NTP_DELTA;
-		time_t epoch = seconds_since_1970;
-		debug_print("second since 1970 = %i\n", epoch);
-		ntp_result(state, NTP_REPLY_RECEIVED, &epoch);
-	} else {
-		ntp_result(state, NTP_BAD_REPLY, NULL);
-	}
-	pbuf_free(p);
-}
+	if ( !state->first && !state->err ) return NTP_RESEND_TIME;
+	state->first = 0;
 
+	cyw43_arch_lwip_begin();
+	err = dns_gethostbyname(NTP_SERVER,
+	                        &state->ntp_server_address,
+	                        ntp_dns_found,
+	                        state);
+	cyw43_arch_lwip_end();
 
-static int64_t ntp_failed_handler(alarm_id_t id, void *user_data)
-{
-	NTP_T *state = (NTP_T*)user_data;
-	ntp_result(state, NTP_TIMEOUT, NULL);
-	return 0;
+	if ( err == ERR_OK ) {
+		/* Immediate reply -> cached result */
+		ntp_request(state);
+		state->err = 0;
+	} else if (err != ERR_INPROGRESS) {
+		state->err = 1;
+	}
+	return NTP_RESEND_TIME;
 }
 
 
-NTP_T *ntp_init(void (*reply_func)(int))
+NTP_T *ntp_init()
 {
 	NTP_T *state = calloc(1, sizeof(NTP_T));
 	if (!state) return NULL;
 
-	state->callback = reply_func;
+	state->err = 0;
+	state->ok = 0;
+	state->first = 1;
 
 	state->ntp_pcb = udp_new_ip_type(IPADDR_TYPE_ANY);
 	if (!state->ntp_pcb) {
 		free(state);
 		return NULL;
 	}
+
+	/* Set up UDP callback */
 	udp_recv(state->ntp_pcb, ntp_recv, state);
+
+	/* It's probably too early to start resolving hostnames or sending
+	 * UDP packets.  Call us back in a moment. */
+	state->send_alarm = add_alarm_in_us(NTP_RESEND_TIME,
+	                                    send_handler,
+	                                    state,
+	                                    true);
+
 	return state;
 }
 
 
-void ntp_send_request(NTP_T *state)
+int ntp_ok(NTP_T *state)
 {
-	if ( state == NULL ) return;
-
-	cyw43_arch_lwip_begin();
-	int err = dns_gethostbyname(NTP_SERVER,
-	                            &state->ntp_server_address,
-	                            ntp_dns_found,
-	                            state);
-	cyw43_arch_lwip_end();
+	if ( state == NULL ) return 0;
+	return state->ok;
+}
 
-	state->ntp_resend_alarm = add_alarm_in_ms(NTP_RESEND_TIME,
-	                                          ntp_failed_handler,
-	                                          state,
-	                                          true);
 
-	state->dns_request_sent = true;
-	if ( err == ERR_OK ) {
-		/* Immediate reply -> cached result */
-		ntp_request(state);
-	} else if (err != ERR_INPROGRESS) {
-		/* Error sending DNS request
-		 * (ERR_INPROGRESS means expect a callback) */
-		ntp_result(state, NTP_DNS_ERROR, NULL);
-	}
+int ntp_err(NTP_T *state)
+{
+	if ( state == NULL ) return 0;
+	return state->err;
 }
diff --git a/ntp_client.h b/ntp_client.h
index f21f546..ac164a8 100644
--- a/ntp_client.h
+++ b/ntp_client.h
@@ -31,18 +31,9 @@ typedef struct NTP_T_ NTP_T;
  * Sorry, no automatic DST handling yet. */
 #define UTC_OFFSET_SEC 3600
 
-enum {
-	NTP_DNS_ERROR,
-	NTP_DNS_NO_ADDR,
-	NTP_REQUEST_SENT,
-	NTP_BAD_REPLY,
-	NTP_TIMEOUT,
-	NTP_REPLY_RECEIVED
-};
-
-extern NTP_T *ntp_init(void (*reply_func)(int));
-extern void ntp_send_request(NTP_T *state);
-extern void ntp_poll(NTP_T *state);
+extern NTP_T *ntp_init(void);
+extern int ntp_ok(NTP_T *state);
+extern int ntp_err(NTP_T *state);
 
 /* For verbose debugging, switch the commenting of these two lines.
  * .. and see CMakeLists.txt */