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.

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

Simplify EEPROM implementation #1

Closed
3 tasks done
mciantyre opened this issue Jan 23, 2023 · 12 comments · Fixed by #3 or #5
Closed
3 tasks done

Simplify EEPROM implementation #1

mciantyre opened this issue Jan 23, 2023 · 12 comments · Fixed by #3 or #5

Comments

@mciantyre
Copy link
Contributor

mciantyre commented Jan 23, 2023

mciantyre/teensy4-rs#86 notes that teensy4-bsp does not support the larger flash available on the Teensy 4.1. The emulated EEPROM prototype in mciantyre/teensy4-rs#100 includes this assumption: the EEPROM prototype exposes 1080 bytes of EEPROM, the max available on a Teensy 4.0.

We could change mciantyre/teensy4-rs#86. But, adding dedicated support for both the Teensy 4.0 and 4.1 boards is tricky to get right. We should take this route if we really need more than 1080 bytes of storage capacity.

On the other hand, if we can stay under 1080 bytes of capacity, we shouldn't need to change anything from the mciantyre/teensy4-rs#100 prototype. This issue tracks this idea. If realized, we can simplify the EEPROM implementation proposed in this repo. But, going forward, we'll need to keep the emulated EEPROM utilization to under 1080 bytes.

@mciantyre
Copy link
Contributor Author

mciantyre commented Jan 23, 2023

Other approaches, thinking out loud:

  • From my understanding, the EEPROM emulation implements wear-leveling. Reducing the space utilized for wear-leveling in the EEPROM C module means we get that capacity back for our own data. I'd need to study the code some more to understand how that's working. Naively, I'd think that reducing the wear-leveling space by half means we would halve the expected storage lifetime. I'd want to know the current expected lifetime before digging deeper (I think it's 100K writes?).
  • Hard-code Teensy 4.1 assumptions throughout the BSP. Should get the job done, but means we can't always build for a 4.0 board. Harder to upstream.

@mciantyre mciantyre self-assigned this Jan 23, 2023
@mciantyre
Copy link
Contributor Author

Document what's stored in emulated EEPROM, and show that it fits within 1080 bytes.

Today's intended storage layout is

  • 500 bytes (emulated EEPROM offsets [0x00, 0x1F3], inclusive) for information common to all applications / devices.
  • Application- / device-specific data starts at EEPROM offset 0x1F4, and may use up to the end of EEPROM.

During our in-person discussion, we looked at an example of an app- / device-specific configuration that would be persisted. Let me know if this is the wrong structure.

I'm realizing something I missed during our in-person: Although rust-analyzer suggests that the size of PPXStaticInput is 40 bytes, that includes the size of String, which is heap allocated. 40 bytes doesn't actually represent the maximum size of that structure, since the storage for String is defined at runtime.

@dstric-aqueduct, do we have an upper-bound on the size of those strings, and do we know how they're serialized before they're written to flash? If we take this approach, I want to make sure we can fit within 580 bytes for device-specific data.

@dstric-aqueduct
Copy link
Collaborator

During our in-person discussion, we looked at an example of an app- / device-specific configuration that would be persisted. Let me know if this is the wrong structure.

This is correct - any stat field for a Device struct is fair game for this data.

I'm realizing something I missed during our in-person: Although rust-analyzer suggests that the size of PPXStaticInput is 40 bytes, that includes the size of String, which is heap allocated. 40 bytes doesn't actually represent the maximum size of that structure, since the storage for String is defined at runtime.

@dstric-aqueduct, do we have an upper-bound on the size of those strings, and do we know how they're serialized before they're written to flash? If we take this approach, I want to make sure we can fit within 580 bytes for device-specific data.

Whoops, missed this too - let's allocate 32 bytes to this field (same for other tubing_material fields, such as in the TRCX or PP devices). In this vein, would make sense to change the String types in the aq-core crates to [char; N] to match the firmware representation, but that's a todo.

@mciantyre
Copy link
Contributor Author

mciantyre commented Jan 23, 2023

Sounds great, thanks!

rg "struct .*Static \{" in the aq-core/aq-devices reveals 12 device structs. PPXStatic is the largest, followed closely by TRCXStatic.

If I actually replace String with [u8; 32] to mimic C-style strings, std::mem::size_of::<PPXStatic>() == 536. (This is worst-case; I'd expect to get 4 bytes back when building that struct for a Teensy 4.) There's no padding required to lay out this memory. Confirmed this by building today's code with static asserts.

With today's code, we should be able to stay under 1080 byte capacity for emulated EEPROM.

@mciantyre
Copy link
Contributor Author

Show that the emulated EEPROM demo from mciantyre/teensy4-rs#100 works on both a Teensy 4.0 and 4.1.

I rebased the prototype from mciantyre/teensy4-rs#100 onto the teensy4-bsp-0.3.0 git tag. It's available in the proto/eeprom branch in this repo. I also updated the eeprom.rs example to provide some logging output, which helps with testing.

You're right; the 4.0 and 4.1 have different persistence behaviors with this example. It seems that the 4.1 loses whatever is persisted once it's reprogrammed, whereas the 4.0 maintains that data. I never noticed the persistence loss during reprogramming until now.

I'm using the command-line loader like this:

teensy_loader_cli -w -v --mcu=TEENSY40

Here's the testing table.

Board \ Test: Data persists after power loss Data persists after reprogramming
4.0
4.1
Expected

"Data persists after power loss": after the eeprom.rs example has run once on a board, removing power then re-applying power does not affect persistence. Both boards perform the same here.

"Data persists after reprogramming": after the eeprom.rs example had been programmed to a board, re-programming eeprom.rs again does not affect persistence. This one is curious; my 4.0 board keeps its state, but my 4.1 board does not.

Are these results consistent with your testing?

@dstric-aqueduct
Copy link
Collaborator

Yep, same behavior I see...hmm

@mciantyre
Copy link
Contributor Author

mciantyre commented Jan 23, 2023

This behavior may be expected. From the HalfKay communication protocol documentation,

HalfKay always preforms a complete erase of all blocks when it receives the first write command.

(Assuming "all blocks" excludes those blocks used for storage.)

If HalfKay knows that it's running on a Teensy 4.1, it would erase the blocks we're using for storage. Those same blocks are reserved for storage on the Teensy 4.0. This could explain why the Teensy 4.0 keeps its data while the 4.1 loses the same information (at the same absolute address).

Just to double-check: Do we need to persist data after reprogramming? If we're already in communication with the device for the programming step, we could use a process like (1) read the value out of storage, (2) program the storage, (3) re-write the value to storage.

Otherwise, looks like specializing the EEPROM driver / runtime for the Teensy 4.1 may be necessary. But before that, I'll make sure that placing the Teensy 4.1 storage address to the expected location fixes the reprogramming behavior.

@dstric-aqueduct
Copy link
Collaborator

dstric-aqueduct commented Jan 23, 2023

Wow...that looks like the issue.

Just to double-check: Do we need to persist data after reprogramming? If we're already in communication with the device for the programming step, we could use a process like (1) read the value out of storage, (2) program the storage, (3) re-write the value to storage.

No, no need to persist data during reprogramming, just power cycles.

(fixed to read power cycles)

@mciantyre
Copy link
Contributor Author

I'll make sure that placing the Teensy 4.1 storage address to the expected location fixes the reprogramming behavior.

With 7db6602 in place, programming then re-programming a Teensy 4.1 with the eeprom.rs demo maintains persistence.

No, no need to persist data during reprogramming, just data cycles.

Perfect. As long as we only need persistence during power cycles, we should be able to move forward with the EEPROM module as-is.

Tomorrow, I'll double-check my test, making sure that the ✅ column remains checked, and I'll document the test procol. If that goes well, I'll simplify the EEPROM integration in this repo.

@dstric-aqueduct
Copy link
Collaborator

Perfect - how does another 10AM meeting work for you? I can do earlier or later if another time is better...

@mciantyre
Copy link
Contributor Author

10AM works for me. I sent a calendar invite yesterday, but let me know if it wasn't received. Happy to reschedule anytime before 330pm.

@mciantyre
Copy link
Contributor Author

I rebased the prototype from mciantyre/teensy4-rs#100 onto the teensy4-bsp-0.3.0 git tag.

My rebase was based on an incorrect guess regarding this fork's origin. This repo appears to be based on commit fc6233a in the upstream project. I've rebased proto/eeprom accordingly. I'll use this as the basis of the EEPROM integration into this fork.

@mciantyre mciantyre linked a pull request Jan 24, 2023 that will close this issue
@mciantyre mciantyre changed the title Simply EEPROM implementation Simplify EEPROM implementation Jan 24, 2023
@mciantyre mciantyre linked a pull request Jan 30, 2023 that will close this issue
@mciantyre mciantyre removed their assignment Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants