Skip to content

Merge intern examples #165

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

Closed
wants to merge 143 commits into from
Closed

Merge intern examples #165

wants to merge 143 commits into from

Conversation

JamesH65
Copy link
Collaborator

I have updated a couple of files per @kilograham requests, but I have not tested any of these changes. They look sane. So from my point of view I think this branch can be merged.

aallan
aallan previously approved these changes Oct 19, 2021
Copy link

@aallan aallan left a comment

Choose a reason for hiding this comment

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

LGTM

int main() {
stdio_init_all();
#if !defined(i2c_default) || !defined(PICO_DEFAULT_I2C_SDA_PIN) || !defined(PICO_DEFAULT_I2C_SCL_PIN)
#warning i2c/mpu6050_i2c example requires a board with I2C pins
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should have said i2c/pa1010d_i2c instead of i2c/mpu6050_i2c ?

@lurch lurch mentioned this pull request Oct 19, 2021
@aallan aallan removed the ready to merge Waiting for final approval. label Oct 19, 2021
@aallan
Copy link

aallan commented Oct 19, 2021

So we use #169 instead of this PR then @lurch? 🤔

@lurch
Copy link
Contributor

lurch commented Oct 19, 2021

So we use #169 instead of this PR then @lurch? 🤔

That's probably a Graham-decision? 🤷‍♂️

@kilograham
Copy link
Contributor

i'm fine with that if they are effectively the same (modulo actually working!)

@aallan
Copy link

aallan commented Oct 19, 2021

i'm fine with that if they are effectively the same (modulo actually working!)

Pick one to merge @kilograham when you merge, and then kill the other I guess!

@lurch
Copy link
Contributor

lurch commented Oct 19, 2021

I just realised that this PR (#165) is targeting master, whereas the other PR (#169) is targeting develop.

@kilograham
Copy link
Contributor

Pick one to merge @kilograham when you merge, and then kill the other I guess!

well which is the latest/greatest? also i saw I think that some half finished microphone example got merged?

@lurch
Copy link
Contributor

lurch commented Oct 21, 2021

well which is the latest/greatest?

As it's a bigger PR number, I guess I can claim that #169 is the "latest", but I'm not going to claim that it's the "greatest". 😉
#165 merges merge-intern-examples into master.
#169 contains all the commits from merge-intern-examples rebased against develop (with merge-conflicts resolved to the best of my abilities), with a few README.adoc tweaks tagged onto the end.

If you want to do the merge-rebasing yourself then feel free, and I can always create a separate PR for the README.adoc tweaks.

also i saw I think that some half finished microphone example got merged?

Yeah that was #148 🎤 Perhaps Alasdair got a bit carried away? 🤷‍♂️

@aallan
Copy link

aallan commented Oct 21, 2021

Yeah that was #148 🎤 Perhaps Alasdair got a bit carried away? 🤷‍♂️

It built so I merged it, wasn't it done? We can revert the merge. Well I can from #165, I don't think I can from #169 that'd be @lurch, and I think he'd have to cherry pick?

@kilograham kilograham dismissed aallan’s stale review October 22, 2021 19:57

don't want this pr merged

@kilograham
Copy link
Contributor

closed in favor of #171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants