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

Specify units for ADC HIL #1032

Merged
merged 7 commits into from Jul 6, 2018

Conversation

Projects
None yet
5 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 26, 2018

Pull Request Overview

This pull request updates the ADC HIL. See #1008 for more details.

Testing Strategy

This pull request was tested by compiling.

TODO or Help Wanted

n/a

Documentation Updated

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

Formatting

  • Ran make formatall.

bradjc added some commits Jun 26, 2018

hil: update ADC hil
Add functions for learning about the ADC resolution and voltage
reference. Also change the samples to be left-justified to make it
easier to use the result.

@bradjc bradjc requested a review from brghena Jun 26, 2018

/// samples. This allows the user of this interface to calculate an actual
/// voltage from the ADC reading.
///
/// The returned reference voltage is in millivolts.

This comment has been minimized.

@phil-levis

phil-levis Jun 26, 2018

Collaborator

is in millivolts, or 0 if unknown.

This comment has been minimized.

@ppannuto

ppannuto Jun 26, 2018

Member

Would it be more "rust"-y to return an Option<usize> and None if unknown?

This comment has been minimized.

@brghena

brghena Jun 26, 2018

Contributor

Either of these seems fine to me. get_resolution_bits should work the same way though.

This comment has been minimized.

@bradjc

bradjc Jun 27, 2018

Contributor

Is it possible that an ADC wouldn't know the resolution of the sample it is taking?

This comment has been minimized.

@brghena

brghena Jun 27, 2018

Contributor

Hmm, why should the reference voltage be optional then? If the suggestion is that the reference could be an external pin that the ADC doesn't know, that could be passed into the driver at init time by the board main.rs configuration.

This comment has been minimized.

@bradjc

bradjc Jun 27, 2018

Contributor
  • That value could instead be passed to whatever would be using get_reference_voltage().
  • If the HW doesn't know, then conceptually it makes sense that it should return that it doesn't know.
  • The reference voltage could change at runtime.

@bradjc bradjc referenced this pull request Jun 28, 2018

Open

RFC: ADC HIL #1008

1 of 2 tasks complete

@alevy alevy changed the title ADC HIL updates Specify units for ADC HIL Jul 4, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

I changed the title of the PR to be more descriptive, but I'd also greatly appreciate a more verbose description of what this PR actually does for posterity. A reference is nice, but I actually had to look at the code to figure out what change this PR decided to actually make.

@alevy alevy added the last-call label Jul 4, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

@phil-levis I think this is what you'd been advocating for, right? Does this seem good?

@ppannuto ppannuto dismissed stale reviews from alevy, brghena, and themself via a32a34d Jul 4, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 4, 2018

Following @alevy's lead from #1044, I've added a CHANGELOG entry for these updates as well. Sadly, which gets merged first will conflict the other, but it should be a quick and easy fixup.

@ppannuto
Copy link
Member

ppannuto left a comment

Re-approve after CHANGELOG merge conflict resolution.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 6, 2018

Blocked on #1078

Merge branch 'master' into adc-hil-updates
Pull in NumCell fix from master

@ppannuto ppannuto merged commit 362baee into master Jul 6, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the adc-hil-updates branch Jul 6, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 6, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 6, 2018

The addition of the CHANGELOG documenting the change came in 48 hours ago, but that's just a bit of descriptive text. All of the HIL changes have been here for 9 days with no comments but approvals. That's past the one-week "no comment = approval" period we have for significant PRs, which is why I merged.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 6, 2018

This needs to have a ReturnCode parameter, in case the sample is bad in some way. This could happen, for example, if you call stop_sampling() when you are in the midst of an I2C operation to an off-board I2C.

I think you probably mean an off-chip ADC, in which case a ReturnCode following the logic discussed in #1052 would probably make sense. But this HIL is not for peripheral ADCs. Is there a case where a ReturnCode would be useful in an on-chip ADC?

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 6, 2018

Should there be separate HILs for on-chip vs. off-chip ADCs? That seems not very hardware independent. I think the one place we agreed this was the right decision was GPIO, because forcing every GPIO operation to be asynchronous would be extremely onerous.

Examples of errors that the callback might want to give you:

  • a sample was missed (LOVR, 38.6.18 in SAM4L data sheet)
  • the ADC was turned off between when the sample started and acquisition started
  • the ADC sample was cancelled before acquisition
@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 11, 2018

Should there be separate HILs for on-chip vs. off-chip ADCs?

I don't know what ADCs look like well enough, but if there is enough of a difference in how the MCU interacts with them, then absolutely YES

That seems not very hardware independent.

It's not. HIL stands for Hardware Interface Layer (not Independence). It's a set of interfaces to very similar hardware.

A sample was missed (LOVR, 38.6.18 in SAM4L data sheet)

OK, totally! This is an overrun basically, because the client ask for too many values or something? I don't know ADCs well enough, but if this isn't an error that can be caught earlier, it seems totally reasonably to put it in the callback.

the ADC was turned off between when the sample started and acquisition started

Wouldn't either the client know it turned it off or the start function be able to immediately return that the ADC is off? Maybe I'm misunderstanding the scenario.

the ADC sample was cancelled before acquisition

Same. The client should know it cancelled the acquisition.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 11, 2018

Hardware Interface Layer (not Independence). It's a set of interfaces to very similar hardware.

Oh! OK.

I guess the general point is that the asynchrony caused by a bus is very similar to that caused by virtualization. In purely asynchronous I/O systems, this means that an error returned in initiation can deferred to the callback.

Wouldn't either the client know it turned it off or the start function be able to immediately return that the ADC is off? Maybe I'm misunderstanding the scenario.

What if more than one capsule has access to this trait? Or what if there is a bug in the client, such that it incorrectly turns off the ADC during a sample? Just as a read/write in POSIX file I/O will return with an error code, you should get a callback with an error code. You don't want to block forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment