Refactor to use boards#59
Conversation
datdenkikniet
left a comment
datdenkikniet
left a comment
There was a problem hiding this comment.
This looks like a good change to have. Some comments!
| pub address_ranges_flash: Option<&'a [AddressRange]>, | ||
| pub main_ram_start: Option<u64>, | ||
| pub main_ram_end: Option<u64>, | ||
| pub xip_sram_start: Option<u64>, |
There was a problem hiding this comment.
I feel like XIP-RAM is a somewhat RP2xxx specific thing. Does it belong in this struct, or can we do without it, somehow? E.g. abstract away whatever meaning we attach it behind some function?
There was a problem hiding this comment.
Probably should abstract it out into some function instead.
|
|
||
| impl BoardInfo for RP2040 { | ||
| fn is_device_board(&self, device: &UsbDevice) -> bool { | ||
| if device.vendor_id == 0x2e8a || device.product_id == 0x0003 { |
There was a problem hiding this comment.
This should probably be an &&
There was a problem hiding this comment.
Actually I don't really think this should be a double &&. Why should we use a double &&?
There was a problem hiding this comment.
I'm referring to the condition in the if statement, not the function. For the function I agree, a double-reference makes very little sense :P
| if device.vendor_id == 0x2e8a || device.product_id == 0x0003 { | |
| if device.vendor_id == 0x2e8a && device.product_id == 0x0003 { |
There was a problem hiding this comment.
Oh 😅, that makes a lot more sense.
|
|
||
| impl BoardInfo for RP2350 { | ||
| fn is_device_board(&self, device: &UsbDevice) -> bool { | ||
| if device.vendor_id == 0x2e8a || device.product_id == 0x000f { |
There was a problem hiding this comment.
This should probably be an &&
| } | ||
|
|
||
| /// This trait helps by allowing for definitions of multiple different boards. | ||
| pub trait BoardInfo { |
There was a problem hiding this comment.
Given that we &dyn BoardInfo everywhere, I'm thinking: would it make more sense to define a GetBoardInfo trait that has a single function returning a concrete struct BoardInfo with the relevant fields (including something like an fn(device: &UsbDevice) -> bool for USB detection) and passing around (references) to that concrete type everywhere instead?
Since the required info is quite concrete and practically none of it actually depends on the concrete instantiation of our impl BoardInfo, I feel like that could work just as well.
It would prevent us from having an implementor of BoardInfo that can do "more runtime magic", but given that all these functions take &self that wouldn't really happen anyways.
Mostly asking because all of these functions only take &self, and unless we plan to support boards that can do some sort of crazy runtime detection with interior mutability, this trait is not much different from a "static trait" that only has fn my_func() -> val functions instead.
There was a problem hiding this comment.
Makes perfect sense to make a trait to get the info! Will do so!
2 participants
This makes it so you just specify the board instead of the family. This will help with implementing the automatic detection logic via usb.