-
Notifications
You must be signed in to change notification settings - Fork 3
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
Looking for Comments: Adopting core::cell for dynamic borrowing in RTIC #2
Open
HarryMakes
wants to merge
6
commits into
master
Choose a base branch
from
mutability
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
astro
approved these changes
Dec 20, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a really good idea to me. It seems to get rid of lifetime hacks.
astro
reviewed
Dec 20, 2020
astro
reviewed
Dec 20, 2020
astro
reviewed
Dec 20, 2020
therealprof
reviewed
Dec 20, 2020
HarryMakes
force-pushed
the
mutability
branch
2 times, most recently
from
December 28, 2020 09:52
14a75c9
to
f786ba6
Compare
This comment has been minimized.
This comment has been minimized.
* tcp_stm32f407 no longer obtains IP address from environment variables.
HarryMakes
force-pushed
the
mutability
branch
from
December 29, 2020 03:55
f786ba6
to
eeea973
Compare
* lib.rs: fix transmission status checking (line 176) * lib.rs: make RAW_FRAME_LENGTH_MAX the same for both RX and TX, for the purpose of setting MAMXFL in controller, as well as setting MTU for smoltcp device * smoltcp_phy.rs: fix missing MTU definition
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Note: the following write-up has not been edited or cleaned up.)
Summary
A total of 3 aspects should be discussed:
SpiEth
andsmoltcp_phy::SmoltcpDevice
for dynamic borrowingMajor modifications
When I was trying to implement this driver in an RTIC-based project (e.g. sinara-hw's Booster), I encountered the issue where the SPI controller
SpiEth
object was needed to be borrowed twice, because thisSpiEth
object and the smoltcp interfacesmoltcp::iface::EthernetInterface
object were instantiated as "late resources" in the RTIC model.SpiEth
was borrowed when instantiatingEthernetInterface
because my original implementation for thesmoltcp::phy::Device
trait is a struct that contains a mutable reference to theSpiEth
object.From what I've observed, when smoltcp is used in RTIC, the interface is more often related or bound to a register block in STM32 like
ETHERNET_DMA
. However, since the Tx and Rx buffers are accessible by a serial interface instead of DMA in our case, a custom class (SpiEth
) binds to the interface instead, and done by the means of mutable reference instead of moving. This prevents me from instantiating the interface in[#init]
.To avoid the restrictive borrowing rule at Rust compile-time, I decided to re-factor the
smoltcp_phy::SmoltcpDevice
struct such that it now contains acore::cell::RefCell
mutable container wrapping the targetSpiEth
. This removes the need to extendSmoltcpDevice
's lifetime to theSpiEth
object. I also cleaned up my code to avoid using trait objects. The code now uses generics and trait bounds for static dispatch - although usingRefCell
to borrow means dynamic borrowing.Minor modifications
When I was developing the RTIC examples, I also became aware of the
CYCCNT
monotonic timer provided by RTIC, whose mechanism is about "adding" clock cycles to a 32-bit clock value. By experimenting with the code, this method would bring indefinite delays, as demonstrated by the time-out feature of the port 1234. For example, the time-out could be delayed by more than 2 seconds:Meanwhile, some other existing RTIC projects that uses smoltcp seem to ignore the delay and simply use
CYCCNT
. Therefore, it would be great to know whether or not SysTick is preferred over RTIC's monotonic timer.