Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix pmem -> make backup_ram_t data members volatile #1135

Merged
merged 3 commits into from
Jun 9, 2023
Merged

fix pmem -> make backup_ram_t data members volatile #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a24f9de
fix pmem -> make backup_ram_t data members volatile
u-foka Jun 8, 2023
e306cdc
rename pmem debug menu to p.mem
u-foka Jun 8, 2023
46d3b1a
p.mem looked strange with capital P
u-foka Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions firmware/application/apps/ui_debug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "radio.hpp"
#include "string_format.hpp"
#include "crc.hpp"

#include "audio.hpp"

Expand Down Expand Up @@ -381,22 +382,35 @@ DebugMenuView::DebugMenuView(NavigationView& nav) {
{"Peripherals", ui::Color::dark_cyan(), &bitmap_icon_peripherals, [&nav]() { nav.push<DebugPeripheralsMenuView>(); }},
{"Temperature", ui::Color::dark_cyan(), &bitmap_icon_temperature, [&nav]() { nav.push<TemperatureView>(); }},
{"Buttons Test", ui::Color::dark_cyan(), &bitmap_icon_controls, [&nav]() { nav.push<DebugControlsView>(); }},
{"Pmem", ui::Color::dark_cyan(), &bitmap_icon_memory, [&nav]() { nav.push<DebugPmemView>(); }},
{"p.mem", ui::Color::dark_cyan(), &bitmap_icon_memory, [&nav]() { nav.push<DebugPmemView>(); }},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@u-foka Sorry for the late comment, but I would like to point out that Persistent Memory is called "P.Memory" in the "P.Memory Mgmt" app name on the Settings page, and personally I would have called it "P.Memory" on the Debug page too, for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

The original Pmem was definitely inconsistent, but this time I looked around and p.mem seemed to be the most common one :)

The app is called P.Memory Mgmt, the wiki page for it is the same, and thats all the references to P.Memory Mgmt I have found :)

Then theres P.MemMgmt in the wiki tree, P.Mem Mgmt on the title, PersistentMemory in the description and 4 p.mem -s on the ui :)

I'm all in for consistency, but I wonder how should it be achieved. Additionally from the UX standpoint I'd put more emphasis on the "Persistent" part somehow to make the purpose more clear. But its probably too long for most places. We come up with a completely different name like Config Store that could fit easier and could also be shortened while retaining the more of the meaning like Cfg. Store or similar, but that would confuse existing users..

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all your comments. Seems it's inconsistent everywhere... Something like "Config Mem" might convey the functionality better, but might be confusing for existing users (or developers since we call it persistent memory in the code). I would lean towards just calling the app "P.Memory" on the Debug page, but it's no big deal and I leave it totally up to you. :-)

});
set_max_rows(2); // allow wider buttons
}

/* 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)) {
static_assert(sizeof(pmem_data) == memory::map::backup_ram.size());

add_children({&text_page, &registers_widget, &text_checksum, &button_ok});
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()) + " 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(data.check_value, 8));
text_checksum2.set("Calculated CRC: " + to_string_hex(pmem_checksum(data.regfile), 8));

button_ok.on_select = [&nav](Button&) {
nav.pop();
Expand Down
7 changes: 4 additions & 3 deletions firmware/application/apps/ui_debug.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ class DebugPmemView : public View {
DebugPmemView(NavigationView& nav);
void focus() override;
bool on_encoder(const EncoderEvent delta) override;
std::string title() const override { return "Pmem"; }
std::string title() const override { return "p.mem"; }

private:
struct pmem_data {
Expand All @@ -282,13 +282,14 @@ class DebugPmemView : public View {

int32_t page{0};

const pmem_data& data;
volatile const pmem_data& data;

Text text_page{{16, 16, 208, 16}};

RegistersWidget registers_widget;

Text text_checksum{{16, 240, 208, 16}};
Text text_checksum{{16, 232, 208, 16}};
Text text_checksum2{{16, 248, 208, 16}};

Button button_ok{
{240 / 3, 270, 240 / 3, 24},
Expand Down
4 changes: 2 additions & 2 deletions firmware/common/portapack_persistent_memory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,8 @@ struct data_t {

struct backup_ram_t {
private:
uint32_t regfile[63];
uint32_t check_value;
volatile uint32_t regfile[63];
volatile uint32_t check_value;

static void copy(const backup_ram_t& src, backup_ram_t& dst) {
for (size_t i = 0; i < 63; i++) {
Expand Down