Workaround for Capture startup hang (#1285)

* Attempt to fix Capture startup hang

* Pump baseband_queue on M4 startup

* Synchronization experiment

* Moved SpectrumCapture member, better hang detection for M0

* Prevent execute from working on members until class has been initialized.

* Formatting

* Remove workaround.

* Rebase on next

firmware/application/apps/capture_app.cpp

@@ -31,7 +31,6 @@ namespace ui {
 
 CaptureAppView::CaptureAppView(NavigationView& nav)
     : nav_{nav} {
-    freqman_set_bandwidth_option(SPEC_MODULATION, option_bandwidth);
     baseband::run_image(portapack::spi_flash::image_tag_capture);
 
     add_children({
@@ -49,18 +48,9 @@ CaptureAppView::CaptureAppView(NavigationView& nav)
         &waterfall,
     });
 
-    /* THE ONE LINE BELOW IS A TEMPORARY KLUDGE WORKAROUND FOR A MYSTERY M4 BASEBAND HANG ISSUE WHEN THE CAPTURE
-       APP IS THE FIRST APP STARTED AFTER POWER-UP, OR THE CAPTURE APP IS RUN AFTER RUNNING REPLAY WITH A HIGH
-       SAMPLE RATE. IT SHOULD NOT BE NECESSARY SINCE sampling_rate IS ALREADY INITIALIZED BY RADIOSTATE BUT
-       APPARENTLY IS ADDING JUST THE RIGHT AMOUNT OF DELAY.  ISSUE DOES NOT AFFECT ALL PORTAPACK UNITS.
-       INVESTIGATION IS ONGOING, SEE ISSUE #1283. */
-    receiver_model.set_sampling_rate(3072000);
-    /* END MYSTERY HANG WORKAROUND. */
-
     field_frequency_step.set_by_value(receiver_model.frequency_step());
     field_frequency_step.on_change = [this](size_t, OptionsField::value_t v) {
         receiver_model.set_frequency_step(v);
-        this->field_frequency.set_step(v);
     };
 
     option_format.set_selected_index(0);  // Default to C16
@@ -68,25 +58,25 @@ CaptureAppView::CaptureAppView(NavigationView& nav)
         record_view.set_file_type((RecordView::FileType)file_type);
     };
 
+    freqman_set_bandwidth_option(SPEC_MODULATION, option_bandwidth);
     option_bandwidth.on_change = [this](size_t, uint32_t base_rate) {
-        sampling_rate = 8 * base_rate;  // Decimation by 8 done on baseband side
-                                        /* base_rate is used for FFT calculation and display LCD, and also in recording writing SD Card rate. */
+        /* base_rate is used for FFT calculation and display LCD, and also in recording writing SD Card rate. */
         /* ex. sampling_rate values, 4Mhz, when recording 500 kHz (BW) and fs 8 Mhz, when selected 1 Mhz BW ... */
         /* ex. recording 500kHz BW to .C16 file, base_rate clock 500kHz x2(I,Q) x 2 bytes (int signed) =2MB/sec rate SD Card  */
-        waterfall.on_hide();
+        auto sampling_rate = 8 * base_rate;  // Decimation by 8 done on baseband side.
 
         /* Set up proper anti aliasing BPF bandwith in MAX2837 before ADC sampling according to the new added BW Options. */
         auto anti_alias_baseband_bandwidth_filter = filter_bandwidth_for_sampling_rate(sampling_rate);
 
+        waterfall.stop();
         record_view.set_sampling_rate(sampling_rate);
         receiver_model.set_sampling_rate(sampling_rate);
         receiver_model.set_baseband_bandwidth(anti_alias_baseband_bandwidth_filter);
-
-        waterfall.on_show();
+        waterfall.start();
     };
 
-    option_bandwidth.set_selected_index(7);  // Preselected default option 500kHz.
     receiver_model.enable();
+    option_bandwidth.set_selected_index(7);  // Preselected default option 500kHz.
 
     record_view.on_error = [&nav](std::string message) {
         nav.display_modal("Error", message);
@@ -98,11 +88,6 @@ CaptureAppView::~CaptureAppView() {
     baseband::shutdown();
 }
 
-void CaptureAppView::on_hide() {
-    waterfall.on_hide();
-    View::on_hide();
-}
-
 void CaptureAppView::set_parent_rect(const Rect new_parent_rect) {
     View::set_parent_rect(new_parent_rect);
 

firmware/application/apps/capture_app.hpp

@@ -39,7 +39,6 @@ class CaptureAppView : public View {
     CaptureAppView(NavigationView& nav);
     ~CaptureAppView();
 
-    void on_hide() override;
     void focus() override;
     void set_parent_rect(const Rect new_parent_rect) override;
 
@@ -54,8 +53,6 @@ class CaptureAppView : public View {
         "rx_capture", app_settings::Mode::RX,
         app_settings::Options::UseGlobalTargetFrequency};
 
-    uint32_t sampling_rate = 0;
-
     Labels labels{
         {{0 * 8, 1 * 16}, "Rate:", Color::light_grey()},
         {{11 * 8, 1 * 16}, "Format:", Color::light_grey()},

firmware/application/baseband_api.cpp

@@ -31,6 +31,16 @@
 
 #include "core_control.hpp"
 
+/* Set true to enable additional checks to ensure
+ * M4 and M0 are synchronized before passing messages. */
+static constexpr bool enforce_core_sync = true;
+
+/* Set true to enable check for baseband messages getting stuck.
+ * This implies the baseband thread is not dequeuing and has probably stalled.
+ * NB: This check adds a small amout of overhead to the message sending code
+ * and may impact application perf if it is sending a lot of messages. */
+static constexpr bool check_for_message_hang = true;
+
 using namespace portapack;
 
 namespace baseband {
@@ -40,8 +50,18 @@ static void send_message(const Message* const message) {
     // another message is present before setting new message.
     shared_memory.baseband_message = message;
     creg::m0apptxevent::assert_event();
-    while (shared_memory.baseband_message)
-        ;
+
+    if constexpr (check_for_message_hang) {
+        auto count = UINT32_MAX;
+        while (shared_memory.baseband_message && --count)
+            /* spin */;
+
+        if (count == 0)
+            chDbgPanic("Baseband Send Fail");
+    } else {
+        while (shared_memory.baseband_message)
+            /* spin */;
+    }
 }
 
 void AMConfig::apply() const {
@@ -276,17 +296,28 @@ void set_spectrum_painter_config(const uint16_t width, const uint16_t height, bo
 
 static bool baseband_image_running = false;
 
-void run_image(const portapack::spi_flash::image_tag_t image_tag) {
+void run_image(const spi_flash::image_tag_t image_tag) {
     if (baseband_image_running) {
         chDbgPanic("BBRunning");
     }
 
     creg::m4txevent::clear();
+    shared_memory.clear_baseband_ready();
 
-    m4_init(image_tag, portapack::memory::map::m4_code, false);
+    m4_init(image_tag, memory::map::m4_code, false);
     baseband_image_running = true;
 
     creg::m4txevent::enable();
+
+    if constexpr (enforce_core_sync) {
+        // Wait up to 3 seconds for baseband to start handling events.
+        auto count = 3'000u;
+        while (!shared_memory.baseband_ready && --count)
+            chThdSleepMilliseconds(1);
+
+        if (count == 0)
+            chDbgPanic("Baseband Sync Fail");
+    }
 }
 
 void shutdown() {

firmware/application/core_control.cpp

@@ -20,21 +20,20 @@
  * Boston, MA 02110-1301, USA.
  */
 
-#include "core_control.hpp"
-
-#include "hal.h"
-
-#include "lpc43xx_cpp.hpp"
-using namespace lpc43xx;
-
-#include "message.hpp"
 #include "baseband_api.hpp"
+#include "core_control.hpp"
+#include "hal.h"
+#include "lpc43xx_cpp.hpp"
 #include "lz4.h"
+#include "message.hpp"
 
 #include <cstring>
 
-void m4_init(const portapack::spi_flash::image_tag_t image_tag, const portapack::memory::region_t to, const bool full_reset) {
-    const portapack::spi_flash::chunk_t* chunk = reinterpret_cast<const portapack::spi_flash::chunk_t*>(portapack::spi_flash::images.base());
+using namespace lpc43xx;
+using namespace portapack;
+
+void m4_init(const spi_flash::image_tag_t image_tag, const memory::region_t to, const bool full_reset) {
+    const spi_flash::chunk_t* chunk = reinterpret_cast<const spi_flash::chunk_t*>(spi_flash::images.base());
     while (chunk->tag) {
         if (chunk->tag == image_tag) {
             const void* src = &chunk->data[0];
@@ -49,9 +48,8 @@ void m4_init(const portapack::spi_flash::image_tag_t image_tag, const portapack:
             LPC_CREG->M4MEMMAP = to.base();
 
             /* Reset M4 core and optionally all peripherals */
-            LPC_RGU->RESET_CTRL[0] = (full_reset) ? (1 << 1)   // PERIPH_RST
-                                                  : (1 << 13)  // M4_RST
-                ;
+            LPC_RGU->RESET_CTRL[0] = (full_reset) ? (1 << 1)    // PERIPH_RST
+                                                  : (1 << 13);  // M4_RST
 
             return;
         }

firmware/application/radio.cpp

@@ -238,6 +238,7 @@ void set_baseband_filter_bandwidth(const uint32_t bandwidth_minimum) {
 
 void set_baseband_rate(const uint32_t rate) {
     portapack::clock_manager.set_sampling_frequency(rate);
+    // TODO: actually set baseband too?
 }
 
 void set_antenna_bias(const bool on) {

firmware/application/ui/ui_spectrum.cpp

@@ -308,13 +308,25 @@ WaterfallView::WaterfallView(const bool cursor) {
 }
 
 void WaterfallView::on_show() {
-    // TODO: Assert that baseband is not shutdown.
-    baseband::spectrum_streaming_start();
+    start();
 }
 
 void WaterfallView::on_hide() {
-    // TODO: Assert that baseband is not shutdown.
-    baseband::spectrum_streaming_stop();
+    stop();
+}
+
+void WaterfallView::start() {
+    if (!running_) {
+        baseband::spectrum_streaming_start();
+        running_ = true;
+    }
+}
+
+void WaterfallView::stop() {
+    if (running_) {
+        baseband::spectrum_streaming_stop();
+        running_ = false;
+    }
 }
 
 void WaterfallView::show_audio_spectrum_view(const bool show) {

firmware/application/ui/ui_spectrum.hpp

@@ -131,11 +131,14 @@ class WaterfallView : public View {
     WaterfallView& operator=(const WaterfallView&) = delete;
     WaterfallView& operator=(WaterfallView&&) = delete;
 
+    // TODO: remove these, use start/stop directly instead.
     void on_show() override;
     void on_hide() override;
 
-    void set_parent_rect(const Rect new_parent_rect) override;
+    void start();
+    void stop();
 
+    void set_parent_rect(const Rect new_parent_rect) override;
     void show_audio_spectrum_view(const bool show);
 
    private:
@@ -147,6 +150,7 @@ class WaterfallView : public View {
 
     WaterfallWidget waterfall_widget{};
     FrequencyScale frequency_scale{};
+    bool running_{false};
 
     ChannelSpectrumFIFO* channel_fifo{nullptr};
     AudioSpectrum* audio_spectrum_data{nullptr};

firmware/application/ui_navigation.cpp

@@ -497,13 +497,8 @@ ReceiversMenuView::ReceiversMenuView(NavigationView& nav) {
         add_items({{"..", Color::light_grey(), &bitmap_icon_previous, [&nav]() { nav.pop(); }}});
     }
     add_items({
-        {
-            "ADS-B",
-            Color::green(),
-            &bitmap_icon_adsb,
-            [&nav]() { nav.push<ADSBRxView>(); },
-        },
-        //{ "ACARS",	Color::yellow(),	&bitmap_icon_adsb,			[&nav](){ nav.push<ACARSAppView>(); }, },
+        {"ADS-B", Color::green(), &bitmap_icon_adsb, [&nav]() { nav.push<ADSBRxView>(); }},
+        //{ "ACARS",	Color::yellow(),	&bitmap_icon_adsb,			[&nav](){ nav.push<ACARSAppView>(); }},
         {"AIS Boats", Color::green(), &bitmap_icon_ais, [&nav]() { nav.push<AISAppView>(); }},
         {"AFSK", Color::yellow(), &bitmap_icon_modem, [&nav]() { nav.push<AFSKRxView>(); }},
         {"BTLE", Color::yellow(), &bitmap_icon_btle, [&nav]() { nav.push<BTLERxView>(); }},

firmware/baseband/event_m4.cpp

@@ -19,21 +19,18 @@
  * Boston, MA 02110-1301, USA.
  */
 
-#include "event_m4.hpp"
-#include "debug.hpp"
-
-#include "portapack_shared_memory.hpp"
-
-#include "message_queue.hpp"
-
 #include "ch.h"
-
+#include "debug.hpp"
+#include "event_m4.hpp"
 #include "lpc43xx_cpp.hpp"
-using namespace lpc43xx;
+#include "message_queue.hpp"
+#include "portapack_shared_memory.hpp"
 
 #include <cstdint>
 #include <array>
 
+using namespace lpc43xx;
+
 extern "C" {
 
 CH_IRQ_HANDLER(MAPP_IRQHandler) {
@@ -61,6 +58,10 @@ void EventDispatcher::run() {
 
     lpc43xx::creg::m0apptxevent::enable();
 
+    // Indicate to the M0 thread that
+    // M4 is ready to receive message events.
+    shared_memory.set_baseband_ready();
+
     while (is_running) {
         const auto events = wait();
         dispatch(events);

firmware/baseband/proc_capture.cpp

@@ -23,9 +23,7 @@
 #include "proc_capture.hpp"
 
 #include "dsp_fir_taps.hpp"
-
 #include "event_m4.hpp"
-
 #include "utility.hpp"
 
 CaptureProcessor::CaptureProcessor() {
@@ -33,9 +31,13 @@ CaptureProcessor::CaptureProcessor() {
     decim_1.configure(taps_200k_decim_1.taps, 131072);
 
     channel_spectrum.set_decimation_factor(1);
+    ready = true;
 }
 
 void CaptureProcessor::execute(const buffer_c8_t& buffer) {
+    if (!ready)
+        return;
+
     /* 2.4576MHz, 2048 samples */
     const auto decim_0_out = decim_0.execute(buffer, dst_buffer);
     const auto decim_1_out = decim_1.execute(decim_0_out, dst_buffer);

firmware/baseband/proc_capture.hpp

@@ -46,9 +46,13 @@ class CaptureProcessor : public BasebandProcessor {
 
    private:
     // TODO: Repeated value needs to be transmitted from application side.
-    size_t baseband_fs = 0;
+    size_t baseband_fs = 3072000;
     static constexpr auto spectrum_rate_hz = 50.0f;
 
+    // HACK: BasebandThread starts immediately and starts sending data to members
+    // before they are initialized. This is a workaround to prevent uninit data problems.
+    bool ready = false;
+
     BasebandThread baseband_thread{baseband_fs, this, NORMALPRIO + 20, baseband::Direction::Receive};
     RSSIThread rssi_thread{NORMALPRIO + 10};
 

firmware/baseband/proc_gps_sim.hpp

@@ -43,7 +43,7 @@ class ReplayProcessor : public BasebandProcessor {
     void on_message(const Message* const message) override;
 
    private:
-    size_t baseband_fs = 0;
+    size_t baseband_fs = 3072000;
     static constexpr auto spectrum_rate_hz = 50.0f;
 
     BasebandThread baseband_thread{baseband_fs, this, NORMALPRIO + 20, baseband::Direction::Transmit};

firmware/baseband/proc_replay.hpp

@@ -42,7 +42,7 @@ class ReplayProcessor : public BasebandProcessor {
     void on_message(const Message* const message) override;
 
    private:
-    size_t baseband_fs = 0;
+    size_t baseband_fs = 3072000;
     static constexpr auto spectrum_rate_hz = 50.0f;
 
     BasebandThread baseband_thread{baseband_fs, this, NORMALPRIO + 20, baseband::Direction::Transmit};

firmware/common/portapack_shared_memory.hpp

@@ -65,6 +65,11 @@ struct SharedMemory {
         uint8_t data[512];
     } bb_data{{{{0, 0}}, 0, {0}}};
 
+    // Set by the M4 to indicate that the baseband app is ready.
+    bool volatile baseband_ready{false};
+    void clear_baseband_ready() { baseband_ready = false; }
+    void set_baseband_ready() { baseband_ready = true; }
+
     uint8_t volatile request_m4_performance_counter{0};
     uint8_t volatile m4_cpu_usage{0};
     uint16_t volatile m4_stack_usage{0};