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

#359 detect decks both by onewire and params #379

Merged
merged 3 commits into from
Aug 26, 2019

Conversation

NicksonYap
Copy link
Contributor

Resolves:

#359

Have tested for LED Ring only

Uses 'or' operator to detect decks so it's a fairly safe change

@krichardsson
Copy link
Contributor

Nice improvement!

Could I suggest that you remove the one wire part and only keep the parameter solution?
The one wire check was simple to implement back in the days but the parameter solution you added is better and I think it would be clearer without the one wire code.

It might also solve the build problems on travis with lines that are too long :-)

@NicksonYap
Copy link
Contributor Author

I originally intended to remove all ow_search(), however @ataffanel mentioned in: #359 (comment) that there may a race condition where not all params are read

I was only able to test for bcLedRing, where it enables the dropdown for LED setting
I'm not sure how the UI is rendered, but it seems fine for the dropdown

I wasn't able to test the others so it was safer to keep the ow_search()

@NicksonYap
Copy link
Contributor Author

I gave the code a look again, it seems all ow_search() are for dropdowns, i suppose if it works for bcLedRing, it will work for others.

I'll resubmit the PR

@krichardsson
Copy link
Contributor

Great!
Looks like you accidentally included the "Lighthouse quality" LED ring effect from the other PR as well :-)

@krichardsson krichardsson merged commit e51e2d5 into bitcraze:master Aug 26, 2019
@krichardsson krichardsson added this to the Next Release milestone Aug 26, 2019
@krichardsson
Copy link
Contributor

@NicksonYap I did some cleaning when I merged this PR.

  1. There was a mix of changes in the pull request and I reverted everything that was not related to detecting decks using parameters instead of OW memory.
  2. When a parameter is read the value is returned as a string. I changed your cast to a bool into a cast to int to make it work.

I think the problems related to mixed content in the PR is related to you using master for the PR. By using separate feature branches for each pull request, as well as separating features into multiple pull requests, each containing one feature, simplifies the handling on our side.

Never the less, thanks for the contribution!

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.

2 participants