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

ws2812 effects library #2215

Merged
merged 5 commits into from
Jan 23, 2018
Merged

ws2812 effects library #2215

merged 5 commits into from
Jan 23, 2018

Conversation

skycoders
Copy link
Contributor

@skycoders skycoders commented Jan 2, 2018

Contributes to idea in #1352.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

This PR adds two new modules: A color library, extracted to an own module because it is helpful for other RGB LEDs as well (e.g. PWM) and a ws2812 effects module.

The color utils provide conversion between RGB and HSV color spaces, and a convenience function to get color wheel values based on an angle.

The ws2812 effects module provides some nice effects for ws2812 LED strips. This is an initial set, and the module itself is probably not perfect, but it does some nice stuff which might be useful for others as well. The effects have been natively implemented because LUA implementations are much too slow. In order to test the module, it must be enabled in the user_modules configuration before building the firmware.

@@ -28,7 +28,8 @@
//#define LUA_USE_MODULES_BME280
//#define LUA_USE_MODULES_BME680
//#define LUA_USE_MODULES_COAP
//#define LUA_USE_MODULES_CRON
//#define LUA_USE_MODULES_COLOR_UTILS
#define LUA_USE_MODULES_CRON
Copy link
Member

Choose a reason for hiding this comment

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

Please disable module again.

@@ -48,7 +49,7 @@
#define LUA_USE_MODULES_MQTT
#define LUA_USE_MODULES_NET
#define LUA_USE_MODULES_NODE
#define LUA_USE_MODULES_OW
//#define LUA_USE_MODULES_OW
Copy link
Member

Choose a reason for hiding this comment

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

Please enable module again.

@@ -68,7 +69,7 @@
//#define LUA_USE_MODULES_SWITEC
// #define LUA_USE_MODULES_TCS34725
//#define LUA_USE_MODULES_TM1829
#define LUA_USE_MODULES_TLS
//#define LUA_USE_MODULES_TLS
Copy link
Member

Choose a reason for hiding this comment

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

Please enable module again.

The color_utils module provides some basic color transformations useful for color LEDs. It is used by the WS2812_EFFECTS module.


## color\_utils.hsv2grb()
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the backslash here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my one editor had an issue without the backslash, but Atom doesn't - I will remove them all.

state->speed = SPEED_DEFAULT;
state->mode_delay = DELAY_DEFAULT;
state->brightness = BRIGHTNESS_DEFAULT;
state->buffer = buffer;
Copy link
Member

@devsaurus devsaurus Jan 2, 2018

Choose a reason for hiding this comment

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

You'll need to luaL_ref() the buffer object to prevent it from being garbage collected after this function exits.

static int ws2812_effects_set_mode(lua_State* L) {

luaL_argcheck(L, state != NULL, 1, LIBRARY_NOT_INITIALIZED_ERROR_MSG);
const int mode = luaL_checkinteger(L, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Quoting luaL_checkoption():

The usual convention in Lua libraries is to use strings instead of numbers to select options.

I know that our firmware is not 100% consistent here, but please think about turning the mode argument into string for the above reason. See file, gpio and other modules for how to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with adjusting this, I just "cloned" the way it is done in the ws2812 module itself (for fading and single/dual mode). This was the closest thing to look at

- `effect_param` is an optional effect parameter. See the effect modes for further explanations. It can be an integer value or a string.

#### Returns
`nil`
Copy link
Member

Choose a reason for hiding this comment

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

Please add an example that shows how to use the functions.

@devsaurus
Copy link
Member

First of all, I like these additions to enhance the RGB capabilities of the firmware.

My main concern is that we're missing a more generic model. So far there's the buffer class being part of the ws2812 module with some basic functions and effects. Now there's another module adding effects to the buffer objects. If this is ever extended for other LED modules it probably will need refactoring on the API level.

On a more specific level: The effects are only applicable to a single stripe at the moment. Adding support for dual mode and up to 8 stripes for the ESP32 needs a different approach. Probably an own effects class where you derive objects for each buffer/stripe from. What are your thoughts here?

@skycoders
Copy link
Contributor Author

I share your thoughts, however I thought, if I don't try to bring this into the firmware somewhen, I will never do so - that is why I initiated the pull request.

Right now, I don't think that the "old" PWM based strips will really need something like this, as they are just too limited. However, I also stumbled upon SK6812, which is similar to APA102, but not identical. Also, having the current buffer objects with RGB(W) values is suboptimal, as they do not allow lossless brightness adjustments. My final hope is to have a buffer object in the HSV value range and perform effects on this buffer, and then have different writers for each LED strip type, and individual strip connected to the chip.

Unfortunately, I am no LUA module pro, and also miss the time to get all this done fast enough. Maybe there is someone who wants to join? I'd also love to look at the ESP32. I have one, but so far didn't manage to build the firmware.

So, before spending too much time in trying to fix the individual comments, it maybe makes sense to get a common understanding of how and where we should go from here. Thanks!

}

val >>= 8;
Copy link
Member

Choose a reason for hiding this comment

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

I think that this was pulled out of the loop to enable smooth fading. Why move it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. That was probably an accident, I did not intend to change this.

@@ -29,7 +29,7 @@
//#define LUA_USE_MODULES_BME680
//#define LUA_USE_MODULES_COAP
//#define LUA_USE_MODULES_COLOR_UTILS
#define LUA_USE_MODULES_CRON
//define LUA_USE_MODULES_CRON
Copy link
Member

Choose a reason for hiding this comment

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

Don't fiddle with the default modules!

@@ -49,7 +49,7 @@
#define LUA_USE_MODULES_MQTT
#define LUA_USE_MODULES_NET
#define LUA_USE_MODULES_NODE
//#define LUA_USE_MODULES_OW
#define LUA_USE_MODULES_OW
Copy link
Member

Choose a reason for hiding this comment

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

dito

@@ -69,7 +69,7 @@
//#define LUA_USE_MODULES_SWITEC
// #define LUA_USE_MODULES_TCS34725
//#define LUA_USE_MODULES_TM1829
//#define LUA_USE_MODULES_TLS
#define LUA_USE_MODULES_TLS
Copy link
Member

Choose a reason for hiding this comment

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

dito

@skycoders
Copy link
Contributor Author

I copied the current DEV branch user_modules.h file and just added the two modules of this PR. This should be OK now hopefully. Never intended to change them.

Copy link
Member

@devsaurus devsaurus left a comment

Choose a reason for hiding this comment

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

Ok from my side and no technical concerns. I recommend to merge this if no further objections arise.

@marcelstoer marcelstoer added this to the 2.2 milestone Jan 21, 2018
@devsaurus devsaurus merged commit ed56d94 into nodemcu:dev Jan 23, 2018
dnc40085 pushed a commit to dnc40085/nodemcu-firmware that referenced this pull request Mar 3, 2018
* ws2812 effects and color utils modules added

* Added documentation for new modules to mkdocs.yml

* changed mode option to string, documentation, default modules fixed

* updated user_modules.h
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Mar 10, 2018
* ws2812 effects and color utils modules added

* Added documentation for new modules to mkdocs.yml

* changed mode option to string, documentation, default modules fixed

* updated user_modules.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants