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

arduino simpletest for the vcnl4200 #2926

Merged
merged 8 commits into from
Nov 18, 2024
Merged

arduino simpletest for the vcnl4200 #2926

merged 8 commits into from
Nov 18, 2024

Conversation

BlitzCityDIY
Copy link
Collaborator

arduino simpletest for the vcnl4200. reads lux, white light and proximity data

uint16_t alsData = vcnl4200.readALSdata();
Serial.print("ALS Data: ");
Serial.print(alsData);
uint16_t whiteData = vcnl4200.readWhiteData();
Copy link
Member

Choose a reason for hiding this comment

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

Add comment:
// Read the raw white sensor data

to match comment on L39

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.

@BlitzCityDIY The refactor for FastLED looks good. I left one optional comment, you may accept or reject it as it does not affect the program itself.

LGTM

@BlitzCityDIY
Copy link
Collaborator Author

@brentru awesome, thanks! that makes sense about the comment, will add.

@brentru
Copy link
Member

brentru commented Nov 18, 2024

Sounds good - comments like this make the loop code appear more symmetrical.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

I looked into the XY mapping stuff a bit. I didn't find a migration guide but I have some guesses that the change made here is not identical in functionality to the old code.

@@ -286,7 +286,7 @@ void swirly()
// blur it repeatedly. Since the blurring is 'lossy', there's
// an automatic trend toward black -- by design.
uint8_t blurAmount = beatsin8(2,10,255);
blur2d( leds, kMatrixWidth, kMatrixHeight, blurAmount);
Copy link
Member

Choose a reason for hiding this comment

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

I know we're both just making guesses here but I think this is going to change the result of blur2d.

I went back into the fastled history to see how this worked before and after:
FastLED/FastLED@b02a481#diff-6b91d9b6efd432b06dc6eb73a57f6a29d64d6b6983c760779f9e86099d8ddafdL465

Previously, blur2d used the XY function provided in the sketch, this was implicit. So the blur would be using the structure of the disco jacket as defined in the Jacket Table.

After this change, the XY function is not used by blur (though it is used elsewhere through the file), but instead a serpentine mapping defined by myXYMap would be used.

I think that constructWithUserFunction() probably needs to be used, something like: XYMap myXYMap = XYMap::constructWithUserFunction(kMatrixWidth, kMatrixHeight, XY)

The result of an incorrect pixel map is that values from one area of the jacket would blur into non-adjacent pixels, making the effect look more muddled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

very good catch and i was concerned about that one. i just pushed an update that uses constructWithUserFunction(). i hooked up two neopixel matrices and a button to test and compare to the project video and i believe that functionality has been retained

change pins that were used for testing back to documented pins
@BlitzCityDIY BlitzCityDIY merged commit 815e584 into main Nov 18, 2024
98 checks passed
@BlitzCityDIY BlitzCityDIY deleted the arduino_vcnl4200 branch November 18, 2024 18:34
@jepler
Copy link
Member

jepler commented Nov 19, 2024

Thanks!

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.

3 participants