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

Add vl53l0x driver - finish work by Afp316 #291 #363

Merged
merged 15 commits into from
Dec 1, 2022

Conversation

tyeth
Copy link
Contributor

@tyeth tyeth commented Nov 12, 2022

This just finishes off the fine work by @afp316 in #291
Rebased off of latest main, amended last commit to remove .pio/.vscode changes, ran clang-format.

@brentru
Copy link
Member

brentru commented Nov 14, 2022

Build failing due to adafruit/Adafruit_MQTT_Library#217

@tyeth
Copy link
Contributor Author

tyeth commented Nov 15, 2022

Not sure how to retrigger workflow, but just read about hash sign-off so here goes:
#sign-off

@brentru
Copy link
Member

brentru commented Nov 15, 2022

I'm not familiar with #sign-off...

On the workflow, https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/actions/runs/3451873666 you should be able to re-run jobs?

Add_vl53l0x_driver_-finish_work_by_Afp316__291·_adafruit_Adafruit_Wippersnapper_Arduino_b23b0ff

@tyeth
Copy link
Contributor Author

tyeth commented Nov 15, 2022

Sadly not available, I think there's a repository or Org setting for allowing workflow runs from pull requests / forks.

image

This was related but maybe not quite the one: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#enabling-workflows-for-forks-of-private-repositories

@tyeth tyeth force-pushed the afp316-add-vl53l0x-driver branch from b23b0ff to c74f572 Compare November 15, 2022 19:29
@tyeth
Copy link
Contributor Author

tyeth commented Nov 15, 2022

Curious, builds happily on some, and on others the same warning seems fatal (or did they just time out?), in the adafruit vl53l0x library about initialisation. https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/actions/runs/3473545620/jobs/5805679630#step:8:75

@brentru
Copy link
Member

brentru commented Nov 15, 2022

-Wmissing-field-initializers is a warning being treated (correctly) as an error with the ESP32 compiler. Please open an issue over on the Adafruit vl53l0x repo, include the warning and log output, tag me, and I'll (or an engineer from adafruit) will fix it.

@tyeth
Copy link
Contributor Author

tyeth commented Nov 15, 2022

Before I do, I'd like to know why does it compiles the build/UF2 image successfully on the other ESP32 builds, like the one I linked the S2-TFT, with the warning still present?
Also how should it be fixed, does the .h file that defines the struct get defaults specified, or does on define only on each initialisation? Is it ever okay to just disable that warning for a line like that initialisation?

@brentru
Copy link
Member

brentru commented Nov 16, 2022

It did pass, but the build warnings were still thrown: https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/actions/runs/3473545620/jobs/5805679630#step:8:73

I'm not sure why at the moment..

Also how should it be fixed, does the .h file that defines the struct get defaults specified, or does on define only on each initialisation

When a struct is initialized, it must contain all members within the brace-enclosed list of initializers

Is it ever okay to just disable that warning for a line like that initialisation?

Not if we can fix it :)

@tyeth tyeth force-pushed the afp316-add-vl53l0x-driver branch from c74f572 to e876fcd Compare November 17, 2022 22:32
@tyeth
Copy link
Contributor Author

tyeth commented Nov 20, 2022

So there was a missing bit in wippersnapper related to the setting of the time of last poll for the proximity sensor type.

Now just another initialisation warning that I'll stick up an issue for or fix eventually, but the sensor works for now on address 29.
Not really happy with the reliability of the sensor in default config so I've selected high accuracy, but may experiment with other libraries too. I think for now it makes sense to leave as is, but long term offer the options of long_range, high_speed and high_accuracy.

The Dev flag in components with the banners is great :)

@brentru brentru self-requested a review November 21, 2022 16:48
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

@tyeth Read over the PR, some comments..

@brentru
Copy link
Member

brentru commented Nov 28, 2022

@tyeth - OK, addressed in adafruit/Adafruit_VL53L0X#59 and released in https://github.com/adafruit/Adafruit_VL53L0X/releases/tag/1.2.1. Arduino Lib manager will pick this up (probably later today) and I will re-kick the CI.

Thanks for being patient.

@brentru
Copy link
Member

brentru commented Nov 30, 2022

@tyeth This is failing clang/doxy (https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/actions/runs/3517128928/jobs/6038336339). Should be ready to go after. Thanks for your patience. I've also fixed the flaky behavior for building the ESP32-Sx platforms via the CI.

@tyeth tyeth force-pushed the afp316-add-vl53l0x-driver branch from 76bd6b9 to 4a2767e Compare December 1, 2022 15:05
@tyeth
Copy link
Contributor Author

tyeth commented Dec 1, 2022

Wow green lights across the board, a more rare and pleasant sight that we might like to admit 📉
Congratulations on the CI work🥇 Just done the doxygen fix so that should be it!

@tyeth tyeth requested a review from brentru December 1, 2022 15:21
@afp316
Copy link
Contributor

afp316 commented Dec 1, 2022 via email

@brentru
Copy link
Member

brentru commented Dec 1, 2022

Great work - thank you both. Will be included in next release.

@brentru brentru merged commit c5dce4b into adafruit:main Dec 1, 2022
@tyeth tyeth deleted the afp316-add-vl53l0x-driver branch January 11, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants