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

systick: Refactoring. #152

Merged
merged 1 commit into from Aug 29, 2014

Conversation

Projects
None yet
4 participants
@dpc
Contributor

dpc commented Aug 28, 2014

This change rewrites systick module a bit to use new ioreg macro, changes systick::setup a bit to eliminate "boolean trap", exposes tenms() to the user, so the user can read and manipulate it.

/// Read ten millisecond calibration value from hardware
pub fn tenms() -> Option<u32> {
let calib = reg::SYSTICK.calib.tenms();
if calib == 0 {

This comment has been minimized.

@bharrisau

bharrisau Aug 28, 2014

Contributor

Or

match reg::SYSTICK.calib.tenms() {
  0 => None,
  val => Some(val)
}
///
/// Arguments:
///
/// * calibration: Timer reload value, or `CALIBRATED` to use the device 10ms
/// calibrated value.
/// * calibration: Calibration mode
/// * enable_irq: true, if IRQ should be initially enabled.

This comment has been minimized.

@bharrisau

bharrisau Aug 28, 2014

Contributor

You aren't accepting this bool anymore?

This comment has been minimized.

@bgamari

bgamari Aug 28, 2014

Contributor

Right, kill the doc comment. I like that you killed the parameter; it makes things nicely more explicit to first configure the peripheral with setup and then enable_irq.

#[allow(missing_doc)]
pub enum CalibrationMode {
Fallback(u32), //= Prefer hard calibrated 10ms,
//= but use this as a fallback

This comment has been minimized.

@bharrisau

bharrisau Aug 28, 2014

Contributor

Could be all on one line? Not sure why you soft wrapped early.

Good to know it works though.

This comment has been minimized.

@bgamari

bgamari Aug 28, 2014

Contributor

Sadly I don't think //= will even work here. We made up that syntax specifically for ioregs!. It might be nice to propose it though.

This comment has been minimized.

@bharrisau

bharrisau Aug 28, 2014

Contributor

Ahh, yes - didn't even think about it not being in an ioreg. I've opened rust-lang/rust#16816, see how it goes.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 28, 2014

Nice, I like using enums this way.

@farcaller

This comment has been minimized.

Member

farcaller commented Aug 28, 2014

I feel that "fallback" is a bit misleading concept. I want to set the systick clock to the exact time value that I know.

There are two cases: either platform has the value for 10ms or not. It's also a bit more complicated as that reference value is only supported on reference clock, so in the end it's up to PT to initialise the systick clocking correctly (i.e. we don't even need to care about pre-defined constant).

I'm fine if "calibrated" source goes away in that case. Systick is kind of reserved for zinc/os internals (scheduler), so it's not really that of user-facing api.

}
/// Read ten millisecond calibration value from hardware
pub fn tenms() -> Option<u32> {

This comment has been minimized.

@bgamari

bgamari Aug 28, 2014

Contributor

bikeshed: perhaps ten_ms would be a tad more readable?

This comment has been minimized.

@bharrisau

bharrisau Aug 28, 2014

Contributor

Does it even need to be public?

This comment has been minimized.

@dpc

dpc Aug 28, 2014

Contributor

Depends. For standalone applications (not using any rust-provided scheduler), it might be useful to check if there's any calibrated value. Is there any cost having it public?

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 28, 2014

A couple of notes above but otherwise LGTM.

@dpc

This comment has been minimized.

Contributor

dpc commented Aug 28, 2014

I feel that leaving ten_ms public, to let user potentially discover calibrated value if it wants it, and killing CalibrationMode is simplest solution that does not sacrifice simplicity.

I'll fix using your comments and push today/tomorrow. Thanks.

@bgamari

This comment has been minimized.

Contributor

bgamari commented Aug 28, 2014

@dpc I agree. systick::setup(systick::tenms().or(fallback)) seems much clearer to me than yet another type.

@dpc dpc force-pushed the dpc:systick branch from 8f11366 to 39b76aa Aug 29, 2014

@dpc

This comment has been minimized.

Contributor

dpc commented Aug 29, 2014

r?

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 29, 2014

Looks good to me. Thanks for your good work.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Aug 29, 2014

@hacknbot: merge

bharrisau added a commit that referenced this pull request Aug 29, 2014

Merge pull request #152 from dpc/systick
systick: Refactoring into ioreg

@bharrisau bharrisau merged commit 75e4deb into hackndev:master Aug 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment