Enhanced Debug -> Peripherals app to allow direct register modification (#1584)

* Add files via upload

* Add files via upload

* Add files via upload

* Add files via upload

* Clang

* Clang

* Tweak position of field on screen

* Reverted text on Write button per reviewer feedback

firmware/application/apps/ui_debug.cpp

@@ -159,11 +159,8 @@ void TemperatureView::focus() {
 /* RegistersWidget *******************************************************/
 
 RegistersWidget::RegistersWidget(
-    RegistersWidgetConfig&& config,
+    RegistersWidgetConfig&& config)
-    std::function<uint32_t(const size_t register_number)>&& reader)
+    : Widget{}, config(std::move(config)), page_number(0) {
-    : Widget{},
-      config(std::move(config)),
-      reader(std::move(reader)) {
 }
 
 void RegistersWidget::update() {
@@ -180,7 +177,7 @@ void RegistersWidget::paint(Painter& painter) {
 void RegistersWidget::draw_legend(const Coord left, Painter& painter) {
     const auto pos = screen_pos();
 
-    for (size_t i = 0; i < config.registers_count; i += config.registers_per_row()) {
+    for (uint32_t i = 0; i < config.registers_count; i += config.registers_per_row()) {
         const Point offset{
             left, static_cast<int>((i / config.registers_per_row()) * row_height)};
 
@@ -197,12 +194,12 @@ void RegistersWidget::draw_values(
     Painter& painter) {
     const auto pos = screen_pos();
 
-    for (size_t i = 0; i < config.registers_count; i++) {
+    for (uint32_t i = 0; i < config.registers_count; i++) {
         const Point offset = {
             static_cast<int>(left + config.legend_width() + 8 + (i % config.registers_per_row()) * (config.value_width() + 8)),
             static_cast<int>((i / config.registers_per_row()) * row_height)};
 
-        const auto value = reader(i);
+        const auto value = reg_read(i);
 
         const auto text = to_string_hex(value, config.value_length());
         painter.draw_string(
@@ -212,19 +209,61 @@ void RegistersWidget::draw_values(
     }
 }
 
+uint32_t RegistersWidget::reg_read(const uint32_t register_number) {
+    if (register_number < config.registers_count) {
+        switch (config.chip_type) {
+            case CT_PMEM:
+                return portapack::persistent_memory::pmem_data_word((page_number * config.registers_count + register_number) / 4) >> (register_number % 4 * 8);
+            case CT_RFFC5072:
+                return radio::debug::first_if::register_read(register_number);
+            case CT_MAX283X:
+                return radio::debug::second_if::register_read(register_number);
+            case CT_SI5351:
+                return portapack::clock_generator.read_register(register_number);
+            case CT_AUDIO:
+                return audio::debug::reg_read(register_number);
+        }
+    }
+    return 0xFFFF;
+}
+
+void RegistersWidget::reg_write(const uint32_t register_number, const uint32_t value) {
+    if (register_number < config.registers_count) {
+        switch (config.chip_type) {
+            case CT_PMEM:
+                break;
+            case CT_RFFC5072:
+                radio::debug::first_if::register_write(register_number, value);
+                break;
+            case CT_MAX283X:
+                radio::debug::second_if::register_write(register_number, value);
+                break;
+            case CT_SI5351:
+                portapack::clock_generator.write_register(register_number, value);
+                break;
+            case CT_AUDIO:
+                audio::debug::reg_write(register_number, value);
+                break;
+        }
+    }
+}
+
 /* RegistersView *********************************************************/
 
 RegistersView::RegistersView(
     NavigationView& nav,
     const std::string& title,
-    RegistersWidgetConfig&& config,
+    RegistersWidgetConfig&& config)
-    std::function<uint32_t(const size_t register_number)>&& reader)
+    : registers_widget{std::move(config)} {
-    : registers_widget{std::move(config), std::move(reader)} {
     add_children({
         &text_title,
         &registers_widget,
         &button_update,
         &button_done,
+        &labels,
+        &field_write_reg_num,
+        &field_write_data_val,
+        &button_write,
     });
 
     button_update.on_select = [this](Button&) {
@@ -237,6 +276,23 @@ RegistersView::RegistersView(
     text_title.set_parent_rect({(240 - static_cast<int>(title.size()) * 8) / 2, 16,
                                 static_cast<int>(title.size()) * 8, 16});
     text_title.set(title);
+
+    field_write_reg_num.set_value(0);
+    field_write_reg_num.on_change = [this](SymField&) {
+        field_write_data_val.set_value(this->registers_widget.reg_read(field_write_reg_num.to_integer()));
+        field_write_data_val.set_dirty();
+    };
+
+    field_write_data_val.on_change = [this](SymField&) {};
+
+    const auto value = registers_widget.reg_read(0);
+    field_write_data_val.set_value(value);
+
+    button_write.set_style(&Styles::red);
+    button_write.on_select = [this](Button&) {
+        this->registers_widget.reg_write(field_write_reg_num.to_integer(), field_write_data_val.to_integer());
+        this->registers_widget.update();
+    };
 }
 
 void RegistersView::focus() {
@@ -376,18 +432,10 @@ DebugPeripheralsMenuView::DebugPeripheralsMenuView(NavigationView& nav) {
     const char* max283x = hackrf_r9 ? "MAX2839" : "MAX2837";
     const char* si5351x = hackrf_r9 ? "Si5351A" : "Si5351C";
     add_items({
-        {"RFFC5072", ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav]() { nav.push<RegistersView>(
+        {"RFFC5072", ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav]() { nav.push<RegistersView>("RFFC5072", RegistersWidgetConfig{CT_RFFC5072, 31, 16}); }},
-                                                                                              "RFFC5072", RegistersWidgetConfig{31, 16},
+        {max283x, ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav, max283x]() { nav.push<RegistersView>(max283x, RegistersWidgetConfig{CT_MAX283X, 32, 10}); }},
-                                                                                              [](const size_t register_number) { return radio::debug::first_if::register_read(register_number); }); }},
+        {si5351x, ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav, si5351x]() { nav.push<RegistersView>(si5351x, RegistersWidgetConfig{CT_SI5351, 96, 8}); }},
-        {max283x, ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav, max283x]() { nav.push<RegistersView>(
+        {audio::debug::codec_name(), ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav]() { nav.push<RegistersView>(audio::debug::codec_name(), RegistersWidgetConfig{CT_AUDIO, audio::debug::reg_count(), audio::debug::reg_bits()}); }},
-                                                                                                    max283x, RegistersWidgetConfig{32, 10},
-                                                                                                    [](const size_t register_number) { return radio::debug::second_if::register_read(register_number); }); }},
-        {si5351x, ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav, si5351x]() { nav.push<RegistersView>(
-                                                                                                    si5351x, RegistersWidgetConfig{96, 8},
-                                                                                                    [](const size_t register_number) { return portapack::clock_generator.read_register(register_number); }); }},
-        {audio::debug::codec_name(), ui::Color::dark_cyan(), &bitmap_icon_peripherals_details, [&nav]() { nav.push<RegistersView>(
-                                                                                                              audio::debug::codec_name(), RegistersWidgetConfig{audio::debug::reg_count(), audio::debug::reg_bits()},
-                                                                                                              [](const size_t register_number) { return audio::debug::reg_read(register_number); }); }},
     });
     set_max_rows(2);  // allow wider buttons
 }
@@ -420,28 +468,14 @@ DebugMenuView::DebugMenuView(NavigationView& nav) {
 
 /* DebugPmemView *********************************************************/
 
-uint32_t pmem_checksum(volatile const uint32_t data[63]) {
-    CRC<32> crc{0x04c11db7, 0xffffffff, 0xffffffff};
-    for (size_t i = 0; i < 63; i++) {
-        const auto word = data[i];
-        crc.process_byte((word >> 0) & 0xff);
-        crc.process_byte((word >> 8) & 0xff);
-        crc.process_byte((word >> 16) & 0xff);
-        crc.process_byte((word >> 24) & 0xff);
-    }
-    return crc.checksum();
-}
-
 DebugPmemView::DebugPmemView(NavigationView& nav)
-    : data{*reinterpret_cast<pmem_data*>(memory::map::backup_ram.base())}, registers_widget(RegistersWidgetConfig{page_size, 8}, std::bind(&DebugPmemView::registers_widget_feed, this, std::placeholders::_1)) {
+    : registers_widget(RegistersWidgetConfig{CT_PMEM, page_size, 8}) {
-    static_assert(sizeof(pmem_data) == memory::map::backup_ram.size());
-
     add_children({&text_page, &registers_widget, &text_checksum, &text_checksum2, &button_ok});
 
     registers_widget.set_parent_rect({0, 32, 240, 192});
 
-    text_checksum.set("Size: " + to_string_dec_uint(portapack::persistent_memory::data_size(), 3) + "  CRC: " + to_string_hex(data.check_value, 8));
+    text_checksum.set("Size: " + to_string_dec_uint(portapack::persistent_memory::data_size(), 3) + "  CRC: " + to_string_hex(portapack::persistent_memory::pmem_stored_checksum(), 8));
-    text_checksum2.set("Calculated CRC: " + to_string_hex(pmem_checksum(data.regfile), 8));
+    text_checksum2.set("Calculated CRC: " + to_string_hex(portapack::persistent_memory::pmem_calculated_checksum(), 8));
 
     button_ok.on_select = [&nav](Button&) {
         nav.pop();
@@ -451,7 +485,7 @@ DebugPmemView::DebugPmemView(NavigationView& nav)
 }
 
 bool DebugPmemView::on_encoder(const EncoderEvent delta) {
-    page = std::max(0l, std::min((int32_t)page_max, page + delta));
+    registers_widget.set_page(std::max(0ul, std::min((uint32_t)page_count - 1, registers_widget.page() + delta)));
 
     update();
 
@@ -463,17 +497,10 @@ void DebugPmemView::focus() {
 }
 
 void DebugPmemView::update() {
-    text_page.set(to_string_hex(page_size * page, 2) + "+");
+    text_page.set(to_string_hex(registers_widget.page() * page_size, 2) + "+");
     registers_widget.update();
 }
 
-uint32_t DebugPmemView::registers_widget_feed(const size_t register_number) {
-    if (page_size * page + register_number >= memory::map::backup_ram.size()) {
-        return 0xff;
-    }
-    return data.regfile[(page_size * page + register_number) / 4] >> (register_number % 4 * 8);
-}
-
 /* DebugScreenTest ****************************************************/
 
 DebugScreenTest::DebugScreenTest(NavigationView& nav)

firmware/application/apps/ui_debug.hpp

@@ -131,9 +131,18 @@ class TemperatureView : public View {
         "Done"};
 };
 
+typedef enum {
+    CT_PMEM,
+    CT_RFFC5072,
+    CT_MAX283X,
+    CT_SI5351,
+    CT_AUDIO,
+} chip_type_t;
+
 struct RegistersWidgetConfig {
-    size_t registers_count;
+    chip_type_t chip_type;
-    size_t register_bits;
+    uint32_t registers_count;
+    uint32_t register_bits;
 
     constexpr size_t legend_length() const {
         return (registers_count >= 0x10) ? 2 : 1;
@@ -174,17 +183,21 @@ struct RegistersWidgetConfig {
 
 class RegistersWidget : public Widget {
    public:
-    RegistersWidget(
+    RegistersWidget(RegistersWidgetConfig&& config);
-        RegistersWidgetConfig&& config,
-        std::function<uint32_t(const size_t register_number)>&& reader);
 
     void update();
 
     void paint(Painter& painter) override;
 
+    uint32_t reg_read(const uint32_t register_number);
+    void reg_write(const uint32_t register_number, const uint32_t value);
+
+    void set_page(int32_t value) { page_number = value; }
+    uint32_t page(void) { return page_number; }
+
    private:
     const RegistersWidgetConfig config;
-    const std::function<uint32_t(const size_t register_number)> reader;
+    uint32_t page_number;
 
     static constexpr size_t row_height = 16;
 
@@ -194,11 +207,7 @@ class RegistersWidget : public Widget {
 
 class RegistersView : public View {
    public:
-    RegistersView(
+    RegistersView(NavigationView& nav, const std::string& title, RegistersWidgetConfig&& config);
-        NavigationView& nav,
-        const std::string& title,
-        RegistersWidgetConfig&& config,
-        std::function<uint32_t(const size_t register_number)>&& reader);
 
     void focus();
 
@@ -208,12 +217,30 @@ class RegistersView : public View {
     RegistersWidget registers_widget;
 
     Button button_update{
-        {16, 256, 96, 24},
+        {16, 280, 96, 24},
         "Update"};
 
     Button button_done{
-        {128, 256, 96, 24},
+        {128, 280, 96, 24},
         "Done"};
+
+    Button button_write{
+        {144, 248, 80, 20},
+        "Write"};
+
+    Labels labels{
+        {{1 * 8, 248}, "Reg:", Color::light_grey()},
+        {{8 * 8, 248}, "Data:", Color::light_grey()}};
+
+    SymField field_write_reg_num{
+        {5 * 8, 248},
+        2,
+        SymField::Type::Hex};
+
+    SymField field_write_data_val{
+        {13 * 8, 248},
+        4,
+        SymField::Type::Hex};
 };
 
 class ControlsSwitchesWidget : public Widget {
@@ -282,17 +309,8 @@ class DebugPmemView : public View {
     std::string title() const override { return "P.Mem Debug"; }
 
    private:
-    struct pmem_data {
-        uint32_t regfile[63];
-        uint32_t check_value;
-    };
-
     static constexpr uint8_t page_size{96};  // Must be multiply of 4 otherwise bit shifting for register view wont work properly
-    static constexpr uint8_t page_max{(portapack::memory::map::backup_ram.size() + page_size - 1) / page_size - 1};
+    static constexpr uint8_t page_count{(portapack::memory::map::backup_ram.size() + page_size - 1) / page_size};
-
-    int32_t page{0};
-
-    volatile const pmem_data& data;
 
     Text text_page{{16, 16, 208, 16}};
 
@@ -307,7 +325,6 @@ class DebugPmemView : public View {
     };
 
     void update();
-    uint32_t registers_widget_feed(const size_t register_number);
 };
 
 class DebugScreenTest : public View {

firmware/application/audio.cpp

@@ -240,6 +240,10 @@ uint32_t reg_read(const size_t register_number) {
     return audio_codec->reg_read(register_number);
 }
 
+void reg_write(const size_t register_number, uint32_t value) {
+    audio_codec->reg_write(register_number, value);
+}
+
 std::string codec_name() {
     return audio_codec->name();
 }

firmware/application/audio.hpp

@@ -59,6 +59,7 @@ class Codec {
     virtual size_t reg_count() const = 0;
     virtual size_t reg_bits() const = 0;
     virtual uint32_t reg_read(const size_t register_number) = 0;
+    virtual void reg_write(const size_t register_number, const uint32_t value) = 0;
 };
 
 namespace output {
@@ -107,6 +108,7 @@ namespace debug {
 
 size_t reg_count();
 uint32_t reg_read(const size_t register_number);
+void reg_write(const size_t register_number, uint32_t value);
 std::string codec_name();
 size_t reg_bits();
 

firmware/application/hw/max2837.hpp

@@ -836,6 +836,7 @@ class MAX2837 : public MAX283x {
     int8_t temp_sense() override;
 
     reg_t read(const address_t reg_num) override;
+    void write(const address_t reg_num, const reg_t value) override;
 
    private:
     spi::arbiter::Target& _target;
@@ -845,8 +846,6 @@ class MAX2837 : public MAX283x {
 
     void flush_one(const Register reg);
 
-    void write(const address_t reg_num, const reg_t value);
-
     void write(const Register reg, const reg_t value);
     reg_t read(const Register reg);
 

firmware/application/hw/max2839.hpp

@@ -695,6 +695,7 @@ class MAX2839 : public MAX283x {
     int8_t temp_sense() override;
 
     reg_t read(const address_t reg_num) override;
+    void write(const address_t reg_num, const reg_t value) override;
 
    private:
     spi::arbiter::Target& _target;
@@ -704,8 +705,6 @@ class MAX2839 : public MAX283x {
 
     void flush_one(const Register reg);
 
-    void write(const address_t reg_num, const reg_t value);
-
     void write(const Register reg, const reg_t value);
     reg_t read(const Register reg);
 

firmware/application/hw/max283x.hpp

@@ -134,6 +134,7 @@ class MAX283x {
     virtual int8_t temp_sense();
 
     virtual reg_t read(const address_t reg_num);
+    virtual void write(const address_t reg_num, const reg_t value);
 };
 
 }  // namespace max283x

firmware/application/hw/rffc507x.hpp

@@ -841,6 +841,7 @@ class RFFC507x {
     void set_gpo1(const bool new_value);
 
     reg_t read(const address_t reg_num);
+    void write(const address_t reg_num, const reg_t value);
 
    private:
     spi::SPI _bus{};
@@ -848,8 +849,6 @@ class RFFC507x {
     RegisterMap _map{default_hackrf_one};
     DirtyRegisters<Register, reg_count> _dirty{};
 
-    void write(const address_t reg_num, const reg_t value);
-
     void write(const Register reg, const reg_t value);
     reg_t read(const Register reg);
 

firmware/application/radio.cpp

@@ -287,6 +287,10 @@ uint32_t register_read(const size_t register_number) {
     return radio::first_if.read(register_number);
 }
 
+void register_write(const size_t register_number, uint32_t value) {
+    radio::first_if.write(register_number, value);
+}
+
 } /* namespace first_if */
 
 namespace second_if {
@@ -295,6 +299,10 @@ uint32_t register_read(const size_t register_number) {
     return radio::second_if->read(register_number);
 }
 
+void register_write(const size_t register_number, uint32_t value) {
+    radio::second_if->write(register_number, value);
+}
+
 int8_t temp_sense() {
     return radio::second_if->temp_sense();
 }

firmware/application/radio.hpp

@@ -65,12 +65,14 @@ namespace debug {
 namespace first_if {
 
 uint32_t register_read(const size_t register_number);
+void register_write(const size_t register_number, uint32_t value);
 
 } /* namespace first_if */
 
 namespace second_if {
 
 uint32_t register_read(const size_t register_number);
+void register_write(const size_t register_number, uint32_t value);
 
 // TODO: This belongs somewhere else.
 int8_t temp_sense();

firmware/common/ak4951.hpp

@@ -862,6 +862,10 @@ class AK4951 : public audio::Codec {
         return read(reg_address);
     }
 
+    void reg_write(const size_t reg_address, const uint32_t value) override {
+        write(reg_address, value);
+    }
+
    private:
     I2C& bus;
     const I2C::address_t bus_address;

firmware/common/portapack_persistent_memory.cpp

@@ -345,6 +345,19 @@ struct backup_ram_t {
         check_value = compute_check_value();
         copy(*this, dst);
     }
+
+    /* Access functions for DebugPmemView */
+    uint32_t pmem_data_word(uint32_t index) {
+        return (index > sizeof(regfile) / sizeof(uint32_t)) ? 0xFFFFFFFF : regfile[index];
+    }
+
+    uint32_t pmem_stored_checksum(void) {
+        return check_value;
+    }
+
+    uint32_t pmem_calculated_checksum(void) {
+        return compute_check_value();
+    }
 };
 
 static_assert(sizeof(backup_ram_t) == memory::map::backup_ram.size());
@@ -413,6 +426,18 @@ void persist() {
 
 } /* namespace cache */
 
+uint32_t pmem_data_word(uint32_t index) {
+    return backup_ram->pmem_data_word(index);
+}
+
+uint32_t pmem_stored_checksum(void) {
+    return backup_ram->pmem_stored_checksum();
+}
+
+uint32_t pmem_calculated_checksum(void) {
+    return backup_ram->pmem_calculated_checksum();
+}
+
 rf::Frequency target_frequency() {
     rf::tuning_range.reset_if_outside(data->target_frequency, target_frequency_reset_value);
     return data->target_frequency;

firmware/common/portapack_persistent_memory.hpp

@@ -281,6 +281,10 @@ bool should_use_sdcard_for_pmem();
 int save_persistent_settings_to_file();
 int load_persistent_settings_from_file();
 
+uint32_t pmem_data_word(uint32_t index);
+uint32_t pmem_stored_checksum(void);
+uint32_t pmem_calculated_checksum(void);
+
 size_t data_size();
 
 bool debug_dump();

firmware/common/wm8731.cpp

@@ -116,6 +116,8 @@ bool WM8731::write(const Register reg) {
 }
 
 bool WM8731::write(const address_t reg_address, const reg_t value) {
+    map.w[reg_address] = value;  // Save data written in case this fn is called from Debug->Peripherals app
+
     const uint16_t word = (reg_address << 9) | value;
     const std::array<uint8_t, 2> values{
         static_cast<uint8_t>(word >> 8),
@@ -124,10 +126,15 @@ bool WM8731::write(const address_t reg_address, const reg_t value) {
     return bus.transmit(bus_address, values.data(), values.size());
 }
 
+/* WM8731 is a write-only device; the read function only returns the value we last wrote */
 uint32_t WM8731::reg_read(const size_t reg_address) {
     return map.w[reg_address];
 }
 
+void WM8731::reg_write(const size_t reg_address, uint32_t value) {
+    write(reg_address, value);
+}
+
 void WM8731::write(const LeftLineIn value) {
     map.r.left_line_in = value;
     write(Register::LeftLineIn);

firmware/common/wm8731.hpp

@@ -405,6 +405,7 @@ class WM8731 : public audio::Codec {
     }
 
     uint32_t reg_read(const size_t reg_address) override;
+    void reg_write(const size_t reg_address, uint32_t value) override;
 
    private:
     I2C& bus;