From 47e95c0c4798fde472b24d05ae5cc6e001ec6099 Mon Sep 17 00:00:00 2001 From: Kyle Reed <3761006+kallanreed@users.noreply.github.com> Date: Sat, 22 Jul 2023 10:06:55 -0700 Subject: [PATCH] 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 | 27 ++++---------- firmware/application/apps/capture_app.hpp | 3 -- firmware/application/baseband_api.cpp | 39 ++++++++++++++++++--- firmware/application/core_control.cpp | 20 +++++------ firmware/application/radio.cpp | 1 + firmware/application/ui/ui_spectrum.cpp | 20 ++++++++--- firmware/application/ui/ui_spectrum.hpp | 6 +++- firmware/application/ui_navigation.cpp | 9 ++--- firmware/baseband/event_m4.cpp | 19 +++++----- firmware/baseband/proc_capture.cpp | 6 ++-- firmware/baseband/proc_capture.hpp | 6 +++- firmware/baseband/proc_gps_sim.hpp | 2 +- firmware/baseband/proc_replay.hpp | 2 +- firmware/common/portapack_shared_memory.hpp | 5 +++ 14 files changed, 100 insertions(+), 65 deletions(-) diff --git a/firmware/application/apps/capture_app.cpp b/firmware/application/apps/capture_app.cpp index 901c6e831..98c09cd67 100644 --- a/firmware/application/apps/capture_app.cpp +++ b/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); diff --git a/firmware/application/apps/capture_app.hpp b/firmware/application/apps/capture_app.hpp index 03ddfcb07..880cd6956 100644 --- a/firmware/application/apps/capture_app.hpp +++ b/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()}, diff --git a/firmware/application/baseband_api.cpp b/firmware/application/baseband_api.cpp index 4aa7720e9..77adb2bb1 100644 --- a/firmware/application/baseband_api.cpp +++ b/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() { diff --git a/firmware/application/core_control.cpp b/firmware/application/core_control.cpp index a037b0e3e..f4237af3d 100644 --- a/firmware/application/core_control.cpp +++ b/firmware/application/core_control.cpp @@ -20,21 +20,20 @@ * Boston, MA 02110-1301, USA. */ +#include "baseband_api.hpp" #include "core_control.hpp" - #include "hal.h" - #include "lpc43xx_cpp.hpp" -using namespace lpc43xx; - -#include "message.hpp" -#include "baseband_api.hpp" #include "lz4.h" +#include "message.hpp" #include -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(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(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; } diff --git a/firmware/application/radio.cpp b/firmware/application/radio.cpp index 2fc79b99c..ea5f07a35 100644 --- a/firmware/application/radio.cpp +++ b/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) { diff --git a/firmware/application/ui/ui_spectrum.cpp b/firmware/application/ui/ui_spectrum.cpp index 2242d5eac..816f3ea35 100644 --- a/firmware/application/ui/ui_spectrum.cpp +++ b/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) { diff --git a/firmware/application/ui/ui_spectrum.hpp b/firmware/application/ui/ui_spectrum.hpp index d46427181..defe618ee 100644 --- a/firmware/application/ui/ui_spectrum.hpp +++ b/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}; diff --git a/firmware/application/ui_navigation.cpp b/firmware/application/ui_navigation.cpp index 1452f1506..101ecfc6b 100644 --- a/firmware/application/ui_navigation.cpp +++ b/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(); }, - }, - //{ "ACARS", Color::yellow(), &bitmap_icon_adsb, [&nav](){ nav.push(); }, }, + {"ADS-B", Color::green(), &bitmap_icon_adsb, [&nav]() { nav.push(); }}, + //{ "ACARS", Color::yellow(), &bitmap_icon_adsb, [&nav](){ nav.push(); }}, {"AIS Boats", Color::green(), &bitmap_icon_ais, [&nav]() { nav.push(); }}, {"AFSK", Color::yellow(), &bitmap_icon_modem, [&nav]() { nav.push(); }}, {"BTLE", Color::yellow(), &bitmap_icon_btle, [&nav]() { nav.push(); }}, diff --git a/firmware/baseband/event_m4.cpp b/firmware/baseband/event_m4.cpp index 98767dc00..e4888dbc1 100644 --- a/firmware/baseband/event_m4.cpp +++ b/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 #include +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); diff --git a/firmware/baseband/proc_capture.cpp b/firmware/baseband/proc_capture.cpp index 8703bcee8..feed7f129 100644 --- a/firmware/baseband/proc_capture.cpp +++ b/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); diff --git a/firmware/baseband/proc_capture.hpp b/firmware/baseband/proc_capture.hpp index 99c48735b..ff71273c5 100644 --- a/firmware/baseband/proc_capture.hpp +++ b/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}; diff --git a/firmware/baseband/proc_gps_sim.hpp b/firmware/baseband/proc_gps_sim.hpp index c0f07192e..ee2a66620 100644 --- a/firmware/baseband/proc_gps_sim.hpp +++ b/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}; diff --git a/firmware/baseband/proc_replay.hpp b/firmware/baseband/proc_replay.hpp index d06e0b06a..ec6bc99f1 100644 --- a/firmware/baseband/proc_replay.hpp +++ b/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}; diff --git a/firmware/common/portapack_shared_memory.hpp b/firmware/common/portapack_shared_memory.hpp index b335b252c..24b84c482 100644 --- a/firmware/common/portapack_shared_memory.hpp +++ b/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};