Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Dec 16, 2019

This PR includes two patches from master, plus #21547, that should disappear when the topic branch is rebased. Only the last commit in the series is specific to the new API.

@zephyrbot
Copy link

zephyrbot commented Dec 16, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pabigot pabigot added the DNM This PR should not be merged (Do Not Merge) label Dec 20, 2019
@pabigot
Copy link
Contributor Author

pabigot commented Dec 20, 2019

Not ready for review, I forgot that gpio_pin_{en,dis}able is to be deprecated and replacing it isn't quite trivial.

@pabigot pabigot requested a review from dbkinder as a code owner December 20, 2019 23:11
@pabigot pabigot removed the DNM This PR should not be merged (Do Not Merge) label Dec 20, 2019
@pabigot
Copy link
Contributor Author

pabigot commented Dec 20, 2019

gpio_pin_{en,dis}able is to be deprecated and replacing it isn't quite trivial.

Very non-trivial. Reworked for master in #21547 which made the conversion trivial.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

I reviewed "drivers: sensor: mcp9808: update to new GPIO API" commit only.

@mnkp
Copy link
Member

mnkp commented Dec 22, 2019

I believe it is too early to remove DNM label since "drivers: sensor: mcp9808: fix various problems and improve test" commit hasn't been merged on master yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's change this to:

maintains a |plusminus| 2 |deg| C window ...

Copy link
Contributor

Choose a reason for hiding this comment

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

These three lines should be

   trigger fired 1, temp 15.9375 Cel
   Alert on temp outside [15437, 16437] mCel
   Trigger set got 0

Copy link
Contributor

Choose a reason for hiding this comment

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

@pabigot I see doc changes in #21547 that are different than these. (Let's make sure the doc changes from this PR win.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll implement them in #21547; this PR is dependent on that one.

These three lines are exactly as produced by the sample, because the output is interleaved because two separate threads produce it. I can add a comment explaining this, but it would be misleading to "fix" the output when it doesn't represent what the user will see.

Copy link
Contributor

@dbkinder dbkinder Dec 24, 2019

Choose a reason for hiding this comment

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

Sigh. Yes, please explain the mangled output because separate threads are writing it (unless there's an API available that prevents interleaving output). Otherwise, it looks like a misspelling.

@pabigot pabigot added the DNM This PR should not be merged (Do Not Merge) label Dec 23, 2019
@pabigot
Copy link
Contributor Author

pabigot commented Dec 23, 2019

DNM until #21547 is merged to master and topic-gpio is rebased on that.

Since this was converted to the setup/handle/process idiom in master
the conversion is straightforward.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot pabigot removed the DNM This PR should not be merged (Do Not Merge) label Jan 16, 2020
@MaureenHelm MaureenHelm dismissed dbkinder’s stale review January 21, 2020 23:28

doc changes no longer part of this PR

@MaureenHelm MaureenHelm merged commit c145ec5 into zephyrproject-rtos:topic-gpio Jan 21, 2020
@pabigot pabigot deleted the gpio/20191216c branch January 25, 2020 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants