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

Rcc improvement #420

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

AndreySmirnov81
Copy link
Contributor

No description provided.

@burrbull burrbull changed the title Rcc improvement Rcc impairment Mar 14, 2022
@AndreySmirnov81
Copy link
Contributor Author

Sorry.
@burrbull, please explain why "impairment"?

@burrbull
Copy link
Member

burrbull commented Mar 14, 2022

Because I don't see any "improvement".

  1. If you delete RCC dependency, you need mark enable & reset with unsafe
  2. What sense of &self requirement? (not sure about this)
  3. steal(). Again.

@AndreySmirnov81
Copy link
Contributor Author

AndreySmirnov81 commented Mar 14, 2022

@burrbull

  1. What is the reason unsafe?
    I think that after switching to using bit-banding, the need for &rcc disappeared.
  2. &self is needed so that only the owner of the corresponding peripheral can do enable and reset for it.
    This will help to avoid mistakes like this:
   pub fn initialize(
      ...
      tim2: pac::TIM2,
      ...
   ) -> Self {
      ...
      let rcc = unsafe { &(*pac::RCC::PTR) };
      pac::TIM1::enable(rcc);
      pac::TIM1::reset(rcc);
      ...
   }
  1. A temporary way out of the situation. In the future, it is necessary to make a similar change in the stm32-usbd create: Usb Peripheral::enable() -> UsbPeripheral::enable(&self).
    Is steal() acceptable here https://github.com/stm32-rs/stm32f3xx-hal/blob/565b43ceab7b8255c122e82f4f1f1f2b3e85f11b/src/serial.rs#L1270 ?
    I believe that using steal() in certain situations is quite acceptable.

@burrbull burrbull changed the title Rcc impairment Rcc improvement Mar 14, 2022
@AndreySmirnov81
Copy link
Contributor Author

Please review and merge.

@AndreySmirnov81
Copy link
Contributor Author

@burrbull
Do you agree with the changes?

@AndreySmirnov81
Copy link
Contributor Author

AndreySmirnov81 commented Oct 7, 2022

@therealprof, @TheZoq2
Please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants