Skip to content

Conversation

@Lucas-Bruder
Copy link
Owner

@Lucas-Bruder Lucas-Bruder commented Jun 30, 2017

Added const to function parameters
Removed rmt_interrupt_num
Changed file structure to allow this to be imported as component

Just looking for review right meow

@Lucas-Bruder
Copy link
Owner Author

@MicroJoe I could use some help getting this example to compile using new file structure (build systems are my weakness :)

At the moment, its not able to find the led_strip header file

@gagath
Copy link

gagath commented Jul 1, 2017

I can look at my configuration on monday and help you migrate this to the new build system. Stay tuned.

@gagath
Copy link

gagath commented Jul 5, 2017

I think you should rename the inc/led_strip/ directory to just include/. Here is a screenshot of my current setup for this lib that is compiling with the new build system:

led_strip

And then when you include the component in a project like I do you can just import it like this:

#include "led_strip.h"

@gagath
Copy link

gagath commented Jul 5, 2017

I also noticed that my component.mk is empty because I am using the default recommended file structure (code at root, include files in inc/ directory). I think you should empty the component.mk file.

}

bool led_strip_set_pixel_rgb(struct led_strip_t *led_strip, uint32_t pixel_num, uint8_t red, uint8_t green, uint8_t blue)
bool led_strip_set_pixel_rgb(struct led_strip_t *led_strip, const uint32_t pixel_num, const uint8_t red, const uint8_t green, const uint8_t blue)
Copy link

Choose a reason for hiding this comment

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

Why apply const to copied values? I think const keyword is only relevant when using pointers so that you tell the users you won't modify the value they pass. Const for uint32_t and other copied values seems pointless to me (because you already can't modify it).

@@ -1,202 +0,0 @@

Copy link

Choose a reason for hiding this comment

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

No more license?

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