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

[driver] rewrite adns9800 #1193

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Jul 14, 2024

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?

  • test Adns9800::captureFrame() in hardware
  • finish update of examples/blue_pill_f103/adns9800
  • cleanup adns9800_data.hpp
  • fix Resumable Functions variant

@TomSaw TomSaw force-pushed the driver/adns9800 branch 2 times, most recently from f1db304 to 6748582 Compare July 14, 2024 20:00
Comment on lines 81 to 86
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;
Copy link
Contributor Author

@TomSaw TomSaw Jul 14, 2024

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...

Comment on lines 329 to 330

return D(buffer);
Copy link
Contributor Author

@TomSaw TomSaw Jul 14, 2024

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.

Copy link
Member

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.

@TomSaw
Copy link
Contributor Author

TomSaw commented Jul 14, 2024

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.

Copy link
Member

@salkinium salkinium left a 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.

powerUp() {
/// @see power-up sequence, datasheet page 20
Cs::reset();
modm::this_fiber::sleep_for(10ns);
Copy link
Member

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).

Copy link
Contributor Author

@TomSaw TomSaw Jul 16, 2024

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.

Copy link
Member

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 Show resolved Hide resolved
src/modm/driver/motion/adns9800.hpp Outdated Show resolved Hide resolved
while(not this->acquireMaster()) modm::this_fiber::yield();
Cs::reset();

closure();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
closure();
std::forward<Closure>(closure)();

#CosCppBullshitAboutPerfectForwarding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aiiieee 🙈

@TomSaw TomSaw force-pushed the driver/adns9800 branch 11 times, most recently from 15c1a45 to f08654c Compare July 23, 2024 18:57
@TomSaw
Copy link
Contributor Author

TomSaw commented Aug 21, 2024

Just wanna tell: I'll finish this in the beginning of September.

@salkinium salkinium removed this from the 2024q3 milestone Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants