tock icon indicating copy to clipboard operation
tock copied to clipboard

Published 20 hours ago • verified publisher icon tock

Define Buzzer HIL and PWM Buzzer Driver

Open TeodoraMiu opened this issue 2 years ago • 6 comments

Pull Request Overview

This pull request changes the way the buzzer driver works. Before this PR, the buzzer syscall driver assumed all buzzer systems work with PWM. This PR:

  • defines a buzzer HIL
  • provides a PWM buzzer driver that implements the buzzer HIL
  • provides a generic buzzer syscall driver
  • uses the driver for the Microbit, clue and acd boards

Testing Strategy

This pull request was tested using a Microbit.

TODO or Help Wanted

Feedback for the buzzer HIL is much appreciated.

Documentation Updated

  • [x] Updated the relevant files in /docs, or no updates are required.

Formatting

  • [x] Ran make prepush.

TeodoraMiu avatar Jul 15 '22 09:07 TeodoraMiu

Before this PR, the buzzer syscall driver assumed all buzzer systems work with PWM.

Do you have an example for a buzzer which does not work with PWM? I I'm not mistaken, this PR only has an implementation for PWM-controlled buzzers, right?

lschuermann avatar Jul 15 '22 15:07 lschuermann

Do you have an example for a buzzer which does not work with PWM? I I'm not mistaken, this PR only has an implementation for PWM-controlled buzzers, right?

You might be right, even tough I can think of having for instance an external device over some bus.

The main idea behind the HIL came from a use case where the buzzer was needed from another capsule.

alexandruradovici avatar Jul 15 '22 15:07 alexandruradovici

I am not necessarily averse to having a buzzer HIL, but I think that there should be a compelling usecase for such peripherals which might be representable by a more general-purpose API. I figure an external chip will use some communication bus, which might be a more appropriate layer of abstraction in the kernel. Would you mind describing your usecase for controlling buzzers from within the kernel?

lschuermann avatar Jul 15 '22 16:07 lschuermann

Why did you remove the support for multiple apps and queuing?

I figured that an app that wants to notify the user using the buzzer wants to do it immediately, not wait after another notification. Therefore the buzzer should play the sound instantly, otherwise it doesn't really serve its purpose.

TeodoraMiu avatar Jul 19 '22 06:07 TeodoraMiu

@bradjc Have your concerns been addressed?

phil-levis avatar Aug 01 '22 23:08 phil-levis

@bradjc Have your concerns been addressed?

No, not really. We currently have a virtualized buzzer that supports multiple apps, and this replaces it with one that makes the userspace app responsible for doing retries. Perhaps we need a new "buzz immediate" syscall or syscall driver that boards that only want immediate buzz (or if the platform can't provide an immediate buzz it returns an error) can use. But I don't see why we would get rid of the current support for queuing in the process.

I also don't see the need to change the name from buzzer_driver.rs to buzzer.rs. buzzer_driver is more clear this is a userspace driver.

bradjc avatar Aug 08 '22 15:08 bradjc

I believe this is now good to go. I changed the driver so that there is a separate syscall for the buzzer to immediately buzz, but I also kept a syscall for queuing buzz commands. I also returned to the old buzzer_driver.rs name.

TeodoraMiu avatar Aug 30 '22 04:08 TeodoraMiu

I believe this is now good to go. I changed the driver so that there is a separate syscall for the buzzer to immediately buzz, but I also kept a syscall for queuing buzz commands. I also returned to the old buzzer_driver.rs name.

@bradjc?

phil-levis avatar Sep 08 '22 22:09 phil-levis

@TeodoraMiu Looks like there are some simple conflicts to resolve.

phil-levis avatar Sep 08 '22 22:09 phil-levis

Great!

phil-levis avatar Sep 12 '22 23:09 phil-levis

bors r+

bradjc avatar Sep 13 '22 14:09 bradjc

Build succeeded:

bors[bot] avatar Sep 13 '22 15:09 bors[bot]