8000 component: update ambient light by bradjc · Pull Request #3234 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

component: update ambient light #3234

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 12, 2022
Merged

component: update ambient light #3234

merged 1 commit into from
Oct 12, 2022

Conversation

bradjc
Copy link
Contributor
@bradjc bradjc commented Sep 20, 2022

Pull Request Overview

This pull request updates the ambient light component to use new static format.

Testing Strategy

travis

TODO or Help Wanted

n/a

Documentation Updated

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

Formatting

  • Ran make prepush.

hudson-ayers
hudson-ayers previously approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor
@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

It seems that before this PR, there were 2 components: Isl29035Component and AmbientLightComponent, and that AmbientLightComponent would setup the isl29035 and then use it to expose the AmbientLight driver, while one would use Isl29035Component only if one did not want to use it with the AmbientLight driver. Now, AmbientLightComponent does not include the setup of Isl29035Component, that has to be done seperately.

I think this change makes sense, however, because it makes it much harder to accidentally call one of the static macros twice, so this looks good to me.

alistair23
alistair23 previously approved these changes Sep 21, 2022
View reviewed changes
@bradjc
Copy link
Contributor Author
bradjc commented Sep 21, 2022

It seems that before this PR, there were 2 components: Isl29035Component and AmbientLightComponent, and that AmbientLightComponent would setup the isl29035 and then use it to expose the AmbientLight driver, while one would use Isl29035Component only if one did not want to use it with the AmbientLight driver. Now, AmbientLightComponent does not include the setup of Isl29035Component, that has to be done seperately.

I think this change makes sense, however, because it makes it much harder to accidentally call one of the static macros twice, so this looks good to me.

Yes. I think this component was originally created when components were new and not as settled. This update makes it consistent with the others.

@bradjc bradjc dismissed stale reviews from alistair23 and hudson-ayers via b495b28 September 22, 2022 03:50
@bradjc bradjc force-pushed the component-ambient-light branch from 095de2b to b495b28 Compare September 22, 2022 03:50
@bradjc bradjc added the blocked Waiting on something, like a different PR or a dependency. label Sep 22, 2022
@bradjc bradjc force-pushed the component-ambient-light branch from b495b28 to c335403 Compare September 26, 2022 17:20
@bradjc bradjc removed the blocked Waiting on something, like a different PR or a dependency. label Sep 26, 2022
@github-actions github-actions bot removed the kernel label Sep 26, 2022
@hudson-ayers
Copy link
Contributor

bors r+

@bors
Copy link
Contributor
bors bot commented Oct 12, 2022

Build succeeded:

@bors bors bot merged commit 1277217 into master Oct 12, 2022
@bors bors bot deleted the component-ambient-light branch October 12, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alistair23 alistair23 alistair23 approved these changes

@phil-levis phil-levis phil-levis approved these changes

@hudson-ayers hudson-ayers hudson-ayers approved these changes

Assignees
No one assigned
Labels
component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bradjc @hudson-ayers @alistair23 @phil-levis
0