Skip to content
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

MCP9808 sensor implementation #1188

Merged
merged 6 commits into from
Feb 8, 2022
Merged

MCP9808 sensor implementation #1188

merged 6 commits into from
Feb 8, 2022

Conversation

RicInNewMexico
Copy link
Member

Added support for the MCP9808 temperature sensor from Adafruit.

Changes passed CI build on my repo, ci-build firmware flashed to T-Beam v1.1 and verified working.

@mc-hamster mc-hamster self-assigned this Feb 8, 2022
@mc-hamster mc-hamster marked this pull request as draft February 8, 2022 03:26
@mc-hamster
Copy link
Member

Approved to run CI actions.

Code review is good.

Before I approve, just curious -- what is firmware size before and after this change?

@RicInNewMexico
Copy link
Member Author

RicInNewMexico commented Feb 8, 2022

Prior: 1723728
After: 1726160

And not sure why your ci-build failed, the "build everything" was successful, it looks like it may be due to changes in #1186 to the ci-build action.

Edited to add: Just re-ran my CI build on my repo (in-sync with all commits to meshtastic-device) just to verify. Results: https://github.com/RicInNewMexico/Meshtastic-device/actions/runs/1800367368

@thebentern
Copy link
Contributor

I'm looking into the ci-build failure. It's definitely something with the new PR artifacts build. That's a brand new addition so apparently it still has a kind or two in it.

@mc-hamster
Copy link
Member

Prior: 1723728 After: 1726160

And not sure why your ci-build failed, the "build everything" was successful, it looks like it may be due to changes in #1186 to the ci-build action.

Edited to add: Just re-ran my CI build on my repo (in-sync with all commits to meshtastic-device) just to verify. Results: https://github.com/RicInNewMexico/Meshtastic-device/actions/runs/1800367368

About 2.5k. Thanks!

@mc-hamster
Copy link
Member

I'm looking into the ci-build failure. It's definitely something with the new PR artifacts build. That's a brand new addition so apparently it still has a kind or two in it.

Are we ok merging this as is or do you want to use this to test the ci?

@thebentern
Copy link
Contributor

I'm looking into the ci-build failure. It's definitely something with the new PR artifacts build. That's a brand new addition so apparently it still has a kind or two in it.

Are we ok merging this as is or do you want to use this to test the ci?

I want to test my CI change. Apparently the Github action context for forks is different than branches within the current repo. Github added a pull_request_target event that you can utilize instead that is supposed to dispatch it as if it originated from the target repo supposedly, so I'm hoping my change resolves this. Otherwise, we'll have to remove the new PR artifact comment step from the ci-build..

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

🤖 Pull request artifacts

file commit
pr1188-firmware-1.2.53.e649bc8.zip 1214414

github-actions bot added a commit that referenced this pull request Feb 8, 2022
@RicInNewMexico
Copy link
Member Author

I'm looking into the ci-build failure. It's definitely something with the new PR artifacts build. That's a brand new addition so apparently it still has a kind or two in it.

Are we ok merging this as is or do you want to use this to test the ci?

I want to test my CI change. Apparently the Github action context for forks is different than branches within the current repo. Github added a pull_request_target event that you can utilize instead that is supposed to dispatch it as if it originated from the target repo supposedly, so I'm hoping my change resolves this. Otherwise, we'll have to remove the new PR artifact comment step from the ci-build..

Yay! Success!

@thebentern thebentern merged commit 856f2f9 into meshtastic:master Feb 8, 2022
@thebentern
Copy link
Contributor

Awesome! Great work on the new sensor support.

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.

Feature Proposal / Request: Adafruit MCP9808 & INA260 (Environmental Measurement Plugin)
3 participants