feat(dfu_ext): Add ext dfu flash - #1030
Conversation
acece96 to
07f8a83
Compare
| /// Requires `rmk-boot.x` to be linked into the firmware binary | ||
| /// (e.g. `-Trmk-boot.x` in `.cargo/config.toml`). | ||
| #[cfg(any(feature = "dfu_rp", feature = "dfu_nrf"))] | ||
| pub fn init_flash_from_linkerscript(flash: FlashType) -> PartitionType { |
There was a problem hiding this comment.
honestly, best I can do is hiding the type behind a FlashType and importing the concrete type depending on feature set.
The problem is, that the flash must be stored in a static for the usb registration, but static can't be generics.
So in order to have this as impl NorFlash the user would have to make this static, which overcomplicates the user facing API imo.
Maybe you have a better idea.
There was a problem hiding this comment.
The current design can be simplified a lot by using NorFlash, and 'static is not needed actually.
We can just have:
pub struct RmkDfuInterface<'d, DFU: NorFlash, STATE: NorFlash> {
central: DfuState<RmkDfuHandler<FirmwareHandler<'d, DFU, STATE, ResetImmediate, BLOCK_SIZE_DFU>>>,
#[cfg(feature = "dfu_split")]
passthrough: [Option<DfuState<RmkDfuHandler<PassthroughDfuHandler>>>; MAX_PASSTHROUGH_ALTS],
#[cfg(feature = "dfu_split")]
num_passthrough: usize,
current_alt: u8,
}
impl<'d, DFU: NorFlash, STATE: NorFlash> RmkDfuInterface<'d, DFU, STATE> {
pub fn new(
dfu: DFU,
state: STATE,
aligned: &'d mut AlignedBuffer<DFU_ALIGN>,
#[cfg(feature = "dfu_split")] num_peripherals: usize,
) -> Self {
const { assert!(STATE::WRITE_SIZE <= DFU_ALIGN) };
let updater = BlockingFirmwareUpdater::new(
FirmwareUpdaterConfig { dfu, state },
&mut aligned.0[..STATE::WRITE_SIZE],
);
// ...
}
}
pub fn mark_booted(state: impl NorFlash, aligned: &mut AlignedBuffer<DFU_ALIGN>);And in the user code it becomes:
let flash = Mutex::<CriticalSectionRawMutex, _>::new(RefCell::new(Nvmc::new(p.NVMC)));
let mut aligned = AlignedBuffer([0; 32]);
rmk::dfu::mark_booted(BlockingPartition::new(&flash, STATE_OFFSET, STATE_SIZE), &mut aligned);
let mut dfu = RmkDfuInterface::new(
BlockingPartition::new(&flash, DFU_OFFSET, DFU_SIZE),
BlockingPartition::new(&flash, STATE_OFFSET, STATE_SIZE),
&mut aligned,
SPLIT_PERIPHERALS_NUM,
);
let storage = async_flash_wrapper(BlockingPartition::new(&flash, STORAGE_OFFSET, STORAGE_SIZE));There was a problem hiding this comment.
The UsbTransport requires things to have static lifetime.
src/usb/mod.rs:
pub struct UsbTransport<'a, D: Driver<'static>> {
and
impl<'a, D: Driver<'static>> UsbTransport<'a, D> {
pub fn new(driver: D, device_config: DeviceConfig<'static>) -> Self {
Only way to do static I think is when the type is known, and it's only known in user code. So we have to shift what we do here into user code. (the let flash_mutex: &'static MutexType = FLASH_CELL.init(Mutex::new(RefCell::new(flash)));)
Like this:
// types only added to make things a bit easier to overlook
type InternalFlashMutex =
Mutex<CriticalSectionRawMutex, RefCell<Flash<'static, FLASH, Blocking, { rmk::dfu::FLASH_SIZE }>>>;
type InternalFlashPartition =
BlockingPartition<'static, CriticalSectionRawMutex, Flash<'static, FLASH, Blocking, { rmk::dfu::FLASH_SIZE }>>;
static FLASH_MUTEX: StaticCell<InternalFlashMutex> = StaticCell::new();
let flash_mutex = FLASH_MUTEX.init(InternalFlashMutex::new(RefCell::new(embassy_rp::flash::Flash::<
_,
embassy_rp::flash::Blocking,
{ rmk::dfu::FLASH_SIZE },
>::new_blocking(p.FLASH))));
let state_partition =
InternalFlashPartition::new(flash_mutex, dfu_flash_layout.state_offset, dfu_flash_layout.state_size);
let flash = async_flash_wrapper(InternalFlashPartition::new(
flash_mutex,
dfu_flash_layout.storage_offset,
dfu_flash_layout.storage_size,
));
I will try to make UsbTransport & co a shorter lifetime, but that is a bigger change.
There was a problem hiding this comment.
I will try to make UsbTransport & co a shorter lifetime, but that is a bigger change.
I tinkered around with this, but I think it only gets worse from here.
So I think we should be going what you proposed, instead of hiding the staticcell complexity behind a function that needs concrete types and therefore must be implemented separately for each.
type ExternalFlash = W25qNorFlash<spi::Spi<'static, peripherals::SPI0, spi::Blocking>, Output<'static>>;
type ExternalPartition = BlockingPartition<'static, CriticalSectionRawMutex, ExternalFlash>;
type InternalFlashMutex =
Mutex<CriticalSectionRawMutex, RefCell<Flash<'static, FLASH, Blocking, { rmk::dfu::FLASH_SIZE }>>>;
type InternalFlashPartition =
BlockingPartition<'static, CriticalSectionRawMutex, Flash<'static, FLASH, Blocking, { rmk::dfu::FLASH_SIZE }>>;
#[embassy_executor::main]
async fn main(_spawner: Spawner) {
static DFU_MUTEX: StaticCell<Mutex<CriticalSectionRawMutex, RefCell<ExternalFlash>>> = StaticCell::new();
let dfu_mutex = DFU_MUTEX.init(Mutex::new(RefCell::new(ext_flash)));
let dfu_partition = ExternalPartition::new(dfu_mutex, 0, dfu_mutex.lock(|c| c.borrow().capacity() as u32));
let dfu_flash_layout = dfu_flash_layout();
static FLASH_MUTEX: StaticCell<InternalFlashMutex> = StaticCell::new();
let flash_mutex = FLASH_MUTEX.init(InternalFlashMutex::new(RefCell::new(embassy_rp::flash::Flash::<
_,
embassy_rp::flash::Blocking,
{ rmk::dfu::FLASH_SIZE },
>::new_blocking(p.FLASH))));
let mut state_partition =
InternalFlashPartition::new(flash_mutex, dfu_flash_layout.state_offset, dfu_flash_layout.state_size);
let flash = async_flash_wrapper(InternalFlashPartition::new(
flash_mutex,
dfu_flash_layout.storage_offset,
dfu_flash_layout.storage_size,
));
rmk::dfu::mark_booted(&mut state_partition);
static DFU_IFACE: StaticCell<rmk::dfu::RmkDfuInterface<ExternalPartition, InternalFlashPartition>> =
StaticCell::new();
let dfu_iface = DFU_IFACE.init(rmk::dfu::RmkDfuInterface::new(dfu_partition, state_partition));
let mut usb_transport =
UsbTransport::new_with_dfu(driver, rmk_config.device_config, dfu_iface).with_host_service(&host_service);
// ...
}The handler is then generic over norflash:
pub struct RmkDfuInterface<'d, DFU: NorFlash, STATE: NorFlash> {
central: DfuState<RmkDfuHandler<FirmwareHandler<'d, DFU, STATE, ResetImmediate, BLOCK_SIZE_DFU>>>,
#[cfg(feature = "dfu_split")]
passthrough: [Option<DfuState<RmkDfuHandler<PassthroughDfuHandler>>>; MAX_PASSTHROUGH_ALTS],
#[cfg(feature = "dfu_split")]
num_passthrough: usize,
current_alt: u8,
}
impl<'d, DFU: NorFlash, STATE: NorFlash> RmkDfuInterface<'d, DFU, STATE> {
/// Build the DFU interface from a DFU download partition and a boot state
/// partition.
pub fn new(dfu: DFU, state: STATE, #[cfg(feature = "dfu_split")] num_peripherals: usize) -> Self {
...There was a problem hiding this comment.
What I don't understand is, why UsbTransport needs dfu_iface? The transport should do the transport thing only, forward the data to the DFU part. So it should not own dfu info.
There was a problem hiding this comment.
Is all comes down to embassy-usb-dfu and embassy-boot do not split partition ownership and usb handling apart.
Ultimately the partitions are owned by the BlockingFirmwareUpdater in embassy-boot.
The BlockingFirmwareUpdater is owned by the FirmwareHandler in embassy-usb-dfu, which in turn is owned (& wrapped) by RmkDfuHandler which is owned by DfuState which does the usb stuff.
Maybe I can split the usb part and the firmware handler apart. DfuState gets a handler that only does forward messages between the usb and the actual firmware handler. The actual one then could live on the stack in main and hold the partitions and do the DFU stuff with them if it's receiving messages.
There was a problem hiding this comment.
The solution is to create separated Partitions for DFU & Storage? The Partition already implements NorFlash if I remember it correctly
For example:
pub type DfuFlash<F> = embassy_sync::mutex::Mutex<CriticalSectionRawMutex, F>;
pub type DfuPartition<'a, F> = embassy_embedded_hal::flash::partition::Partition<'a, CriticalSectionRawMutex, F>;
pub fn partitions_from_linkerscript<F: NorFlash>(flash: &DfuFlash<F>) -> (DfuPartition<'_, F>, DfuSession<'_, F>) {
use embassy_embedded_hal::flash::partition::Partition;
unsafe extern "C" {
static __bootloader_state_start: u8;
static __bootloader_state_end: u8;
static __bootloader_dfu_start: u8;
static __bootloader_dfu_end: u8;
static __bootloader_storage_start: u8;
static __bootloader_storage_end: u8;
}
let part = |start: &u8, end: &u8| {
let addr = |s: &u8| core::ptr::from_ref(s) as usize as u32;
let (start, end) = (addr(start), addr(end));
Partition::new(flash, start, end - start)
};
// SAFETY: linker-defined absolute symbols — reading their addresses is safe.
unsafe {
(
part(&__bootloader_storage_start, &__bootloader_storage_end),
DfuSession::new(
part(&__bootloader_dfu_start, &__bootloader_dfu_end),
part(&__bootloader_state_start, &__bootloader_state_end),
),
)
}
}
// user code
let dfu_flash = rmk::dfu::DfuFlash::new(rmk::storage::async_flash_wrapper(Nvmc::new(p.NVMC)));
let (flash, mut dfu_session) = rmk::dfu::partitions_from_linkerscript(&dfu_flash);
There was a problem hiding this comment.
DFU and Storage partitions have always been separate.
The solution is to separate USB handling for DFU and writing to the partitions.
Before: one handler does everything, usb requires that handler to be static, therefore everything that handler holds must be static. The handler holds the partition, so they had to be static.
Now: one handler does the USB handling for DFU, but all it does is piping the DFU commands into a channel. Another handler reads from the channel and does the flash operations for DFU and therefore holds the partitions.
Via the channel in the middle the lifetimes of both structs and therefore of the usb handler and the partitions are separated.
Have a look at one of the examples, they are pretty close now to what you proposed in the first place.
I think having a function that creates all the partitions directly from the linkerscript is also possible. Apart from the DFU partition in external flash, because that needs its own flash mutex.
8b62835 to
1293ff6
Compare
4820472 to
cb4419b
Compare
|
Alright taking inspiration from the peripheraldfuhandler and its passthroughhandler I split the dfustate into two separate structs. The first only forwards dfu commands into a channel it is the usb handler and has static lifetime, the other holds the partitions and doesn't need static lifetime. It blocks on messages from that channel and does the writing into partitions, mark updated and such. Only downside it that some has to run that handler with the partitions now but I think that's fair enough. |
b02818d to
40de1a5
Compare
40de1a5 to
0c14322
Compare
Size Report
|
a769f40 to
93200eb
Compare
|
I just found out I am lacking nrf52833 hardware to test this out. Because the BBC Microbit I bought is nrf52833, but it does not have a USB connector. The one USB socket is has is for the on board debugging chip apparently. |
c5f6390 to
d223fed
Compare
Signed-off-by: Pascal Jäger <pascal.jaeger@leimstift.de>
d223fed to
fd5c0a5
Compare
|
I think this is ready now. The one failing CI is because of the version mismatch of crate and what the test expects because you just released. The noswap and nrf52833 path are just changes in the docs for RMK, that's why I included them in here. All users need to do is use the fitting memory.x If someone happens to know a fitting w25q driver I would prefer the user to include it not to have it in this repo. I haven't found one. |
2 participants
Add possibility to use external flash as DFU partition.
Also a partial rewrite of DFU architecture. I split the struct dfustate into two separate structs. The first struct,
UsbProxyDfuHandleronly forwards DFU commands into a channel. This struct is the USB handler and has static lifetime. The second struct,RmkDfuInterfaceholds the partitions and doesn't need static lifetime. It blocks on messages from that channel and does the writing into partitions, mark updated and such. The result is that partitions do not need to be in static cells.Only downside it that someone has to run that handler with the partitions now but I think that's fair enough.
Tests I need to carry out on hardware:
nrf52833nrf52833nrf52833nrf52833nrf52833,noswapnrf52833,dfu_extnrf52833,dfu_extnrf52833,dfu_extnrf52833,dfu_extnrf52840nrf52840nrf52840nrf52840nrf52840,noswapnrf52840,dfu_extnrf52840,dfu_extnrf52840,dfu_extnrf52840,dfu_extrp2040,rp2040-2mbrp2040,rp2040-2mbrp2040,rp2040-2mbrp2040,rp2040-2mbrp2040,rp2040-2mb,dfu_extrp2040,rp2040-2mb,dfu_extrp2040,rp2040-2mb,dfu_extrp2040,rp2040-2mb,dfu_ext