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

fixes #597: Add ledmode to support an uploaded light track #598

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

dolfje
Copy link
Contributor

@dolfje dolfje commented Jun 19, 2020

This adds a ledmode to upload a led sequence that you want to display on the drone. Whenever you enable that ledmode it will go through that uploaded sequence.

@krichardsson
Copy link
Contributor

Thanks for the pull request!

I like the idea of a sequencer for the LED ring and I think the implementation looks OK, but there are some minor things I would like to fix before accepting it.

There is a group of things are related to the size of the ledringtimingsmem.

First of all it is better to use a constant/define, especially since the magic number 3000 is duplicated in two files.

Secondly I would like to have a smaller default size of the memory. RAM is a limited and shared resource and as long as we use static memory allocation for this type of applications (see #579 for more info) I think it is better to keep it fairly small. I think 50 events could be a reasonable size, it would enable users to try it out, but those that want to do more advanced sequences will have to modify the firmware.

You could set the size through a define that can be set from the config.mk file or command line at build time to make it easy to build a FW with larger sequence memory without modifying the code.

I noted that you are using 3 bytes for each event in the ledtimings type. The compiler will word align the structs and use 4 bytes/event anyway, so if you want to you can use the fourth byte for something useful.

BTW we don't require a github issue for a pull request. The pull request it self is an issue and we're happy to use that as it is. There is no harm in creating an issue first though :-)

Thanks!

@dolfje
Copy link
Contributor Author

dolfje commented Jul 15, 2020

The feature is now code-complete and we use the extra bit to support fading, rotation and choosing what leds that should change.
Also an update to the library: bitcraze/crazyflie-lib-python#158

@knmcguire
Copy link
Contributor

I would like to try this out for review, but it is probably better to first fix the python lib first :) I have a comment on that pull request (bitcraze/crazyflie-lib-python#158)

@dolfje
Copy link
Contributor Author

dolfje commented Jul 16, 2020

The lib is already fixed, was just some styling issues (length of the line to big)

@krichardsson krichardsson merged commit e172510 into bitcraze:master Jul 27, 2020
@krichardsson
Copy link
Contributor

Merged

I fixed a problem with the memory mapping. The OW_FIRST_ID must be the last ID in the list for it to work, which might not be obvious.

Thanks!

@krichardsson krichardsson added this to the next-release milestone Jul 27, 2020
cafeciaojoe pushed a commit to cafeciaojoe/crazyflie-firmware that referenced this pull request Sep 27, 2024
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