-
Notifications
You must be signed in to change notification settings - Fork 143
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
[driver] rewrite adns9800 #1193
base: develop
Are you sure you want to change the base?
Conversation
f1db304
to
6748582
Compare
template<int Cpi> | ||
requires (Cpi >= 200) and (Cpi % 200 == 0) and (Cpi <= 8200) | ||
struct Resolution: public modm::Register8 { | ||
Resolution() : modm::Register8(Cpi / 200) {}; | ||
}; | ||
using Resolution_t = modm::Register8; |
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.
Instead of writing all the options from 200, 400, 600, 800, ... , 8200 i came up with this.
It constraints allowed values without overloading the corresponding Adns9800::set(...)
.
Alternatively, something with constexpr
would allow both, compile time and runtime scaling by 1/200 of the argument. let's see...
|
||
return D(buffer); |
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.
EDITED AGAIN:
I've crossed modm's default here to add support for multiple Data
types.
Problem is, the binding of Data&
to the driver class limits the programmer to one type or multiple, symmetric driver instances 😖. (Is this some kind of RF relict?)
Handling Data
exclusively via the emitting method Device::read(...)
feels better:
Either a fresh constructed Data is returned from Device::read(...) or - as a compromise to the current default - a Data&
is passed to Device::read(...)
.
With the "Data&
in Driver
"-design, responsibility for the integrity of Driver::Data&
s is on Driver
due to encapsulation principles -> A mutex on Driver::Data& data
within Driver::read(...)
might become fundamentally at some point!?
In contrast, when returning newly constructed Data
, the API user could take appropriate measures. Returning Data
also facilitates RVO / Copy Elision in typical situations, f.e. when forwarding Data
samples from Driver::read(...)
into a Container
for further processing.
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.
Historically, this was optimizaed for placing all data objects into a big struct that was then serialized directly over a comms network. However this is not how you would do it today anymoe.
Feel free to completely refactor it to your liking.
Not intended to be a dizz, but i've stroken Sascha Schade from the copyright: There's nothing left from the previous driver-port but the lbuild firmware select option. |
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.
I'll review some more next week.
src/modm/driver/motion/adns9800.hpp
Outdated
powerUp() { | ||
/// @see power-up sequence, datasheet page 20 | ||
Cs::reset(); | ||
modm::this_fiber::sleep_for(10ns); |
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.
Use modm::delay(10ns)
. The fiber sleep is only implemented for microseconds, since a context switch takes around 1us. (for comparison one cycle at 100MHz = 10ns-1.) (even on 550MHz STM32H7 it takes ~250ns).
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.
Thought of just some registers to rotate on context switches but 1us(!) sounds like there's more 😅. Assembler is not (yet) in my repertoire but now I feel the need to digg deeper into fibers gearwork.
To fix this pitfall sleep_for(...)
could branch to delay(...)
for durations below an appropriate threshold. With the MCUs clock by hand, this could even be scaled as needed.
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.
Yes, I agree those details should be all taken care of by the function itself.
Have to think about this some more, for example, on AVRs, a context switch takes 10us, so if you sleep for just 1us in a loop, it'll never yield…
(The context switch code is here btw)
src/modm/driver/motion/adns9800.hpp
Outdated
while(not this->acquireMaster()) modm::this_fiber::yield(); | ||
Cs::reset(); | ||
|
||
closure(); |
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.
closure(); | |
std::forward<Closure>(closure)(); |
#CosCppBullshitAboutPerfectForwarding
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.
aiiieee 🙈
15c1a45
to
f08654c
Compare
Just wanna tell: I'll finish this in the beginning of September. |
The Adns9800-driver was made of pure blocking SPI calls - fixed that, added some features and eventually were directed to some clean, fibers-only implementation.
Writing a driver without the RF limitations was a pleasure!
By this chance I wanna ask, if there's a (solid) concept/thoughts how we transition drivers to fibers?Adns9800::captureFrame()
in hardwareexamples/blue_pill_f103/adns9800
adns9800_data.hpp