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

Add basic FASTLED function initated by Joen #433

Merged
merged 23 commits into from Aug 2, 2019

Conversation

@BigDi
Copy link
Contributor

commented Jul 30, 2019

I take the whole pull request from Joen and merge it into the current dev branch. From my point of view some function are missed (e.g. drive a single LED in a stripe for creating a status display. I will add this next time)

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

I think the animation should be non blocking. So instead of using delay() for timing, use millis() and a simple state machine for timing. This way the main loop is not blocked and while animation is running.

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I will clean up the code the create a new pull request.

@BigDi BigDi closed this Jul 30, 2019

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@Legion2 I updated my code but not tested on hardware. But you can already take a look into the code and give some comments. ;-)

@joen

This comment has been minimized.

Copy link

commented Jul 30, 2019

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

@BigDi you don't have to close a PR to update the code. With a PR it's easy to discuss the code changes and keep everything in one place.

@BigDi BigDi reopened this Jul 30, 2019

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Sorry. First time at GitHub. How can I update the Pull Request?

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

it is automatically updated as you pushed to your branch

@Legion2
Copy link
Collaborator

left a comment

some of the code is unreadable because of the code style

"recommendations": [
"platformio.platformio-ide"
]
}

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

remove the vscode settings from git, you can keep it local, but don't commit them

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

I will remove it

if (currentMillis - previousMillis >= blinkInterval)
{
// save the last time you blinked the LED
previousMillis = currentMillis;

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

this will not give a constant blink interval

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

I have no better idea at the moment. Do you have a better idea?

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator
long lastUpdate = 0;
long currentUpdate = 0;

//returns the current step of the animation
int animation_step(int duration, int steps) {
	int currentStep = ((currentUpdate % duration) / ((float)duration)) * steps;
	return currentStep;
}

//returns the number of steps since the last update
int animation_step_count(int duration, int steps) {
	long lastAnimationNumber = lastUpdate / duration;
	long currentAnimationNumber = currentUpdate / duration;
	int lastStep = ((lastUpdate % duration) / ((float)duration)) * steps;
	int currentStep = ((currentUpdate % duration) / ((float)duration)) * steps;

	return currentStep - lastStep + (currentAnimationNumber - lastAnimationNumber) * steps;
}

void loop() {
	lastUpdate = currentUpdate;
	currentUpdate = millis();

	// blink led
	int count = animation_step_count(600, 2);
	if (count > 0) {
		int step = animation_step(600, 2);
		if (step == 0) {
			//set led on
		} else {
			//set led off
		}
	}
}

the animation_step and animation_step_count are from one of my other projects

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

you can get the current animation step, animation steps since you last updated the leds and you can also detect if the animation finished since last updated the leds. You only have to specify the duration and the number of steps you animation should have. The animations run in a loop, so after duration milliseconds the animation step start by 0 again.

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

should be "600" of animation_step_count(600,2); the same like duration?

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

yes the first argument is the duration and the second the step count

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

ah yes i see, updated my comment

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

I updated "my" code...


default:
ledColorBlink[i] = leds[i];
leds[i] = CRGB::Black;

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

can you use a formatter, to fix the indentation of the code. currently it's hard to read the code.

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

I will do a code format.

//#define FASTLED_TYPE LPD8806, DATA_PIN, CLOCK_PIN, RGB>(leds, NUM_LEDS);
//#define FASTLED_TYPE P9813, DATA_PIN, CLOCK_PIN, RGB>(leds, NUM_LEDS);
//#define FASTLED_TYPE APA102, DATA_PIN, CLOCK_PIN, RGB>(leds, NUM_LEDS);
//#define FASTLED_TYPE DOTSTAR, DATA_PIN, CLOCK_PIN, RGB>(leds, NUM_LEDS);

This comment has been minimized.

Copy link
@Legion2

Legion2 Jul 30, 2019

Collaborator

Do we have to predefine led types? This all comes from FastLED and is well documented there.

This comment has been minimized.

Copy link
@BigDi

BigDi Jul 30, 2019

Author Contributor

I can remove the lines. But if someone have other than neopixel he has to added it manually. I have only take over the code from Joen.

BigDi and others added some commits Jul 30, 2019

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

you should not delete files in the .github folder, because they are important for this github repo, like the issue templates and settings.

BigDi added some commits Jul 30, 2019

BigDi
BigDi

@1technophile 1technophile self-requested a review Jul 30, 2019

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

There are some limitations about FastLED and ESP8266 https://github.com/FastLED/FastLED/wiki/ESP8266-notes

@1technophile

This comment has been minimized.

Copy link
Owner

commented on platformio.ini in b3996b5 Jul 30, 2019

You removed accidentaly
esp32dev, uno, atmega

BigDi
BigDi
@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@BigDi regarding main.ino the formatting make it difficult to read the applied changes.
Could you undo the formatting applied to the main.ino file so as to concentrate on your modifications.

Legion2 ask for code formatting and I do it also in main.ino. I take the version from master branch und add the changes again. An undo was not possible because I worked on different machines.

@1technophile

This comment has been minimized.

Copy link
Owner

commented Jul 31, 2019

If you take the version from the master branch you will not have the good one. So as to apply only your changes you can :

  • take the main.ino from the development branch
  • apply your modifications
  • commit them

Legion2 asked for the code formating on a special part of the code that you added. I don't think he asked on all the code.

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

The main.ino should be formatted, but not in this PR. @1technophile you should apply a formatter on all files in an extra PR, after closing most of the open PRs. See #385 for details.

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Which branch is selected when I use the web site of GitHub? I used the "raw" text from the web.

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

development is the default branch

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

So the main.ino is the "right" one.

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

yes

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

the current diff looks right, so i don't think you have to do anything else

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Then I am calmed down for the moment. As I told, my first colaboration work at GitHub...

@Legion2
Copy link
Collaborator

left a comment

how to change the animation type with the Json request?
should the animations be extensible, so you can easily add you own animations without changing the whole code?

main/ZactuatorFASTLED.ino Outdated Show resolved Hide resolved
main/ZactuatorFASTLED.ino Show resolved Hide resolved
main/ZactuatorFASTLED.ino Outdated Show resolved Hide resolved
main/ZactuatorFASTLED.ino Show resolved Hide resolved
main/ZactuatorFASTLED.ino Outdated Show resolved Hide resolved
main/ZactuatorFASTLED.ino Show resolved Hide resolved
main/ZactuatorFASTLED.ino Outdated Show resolved Hide resolved
main/ZactuatorFASTLED.ino Outdated Show resolved Hide resolved

BigDi and others added some commits Jul 31, 2019

BigDi
BigDi
Update main/ZactuatorFASTLED.ino
Do not change it?

Co-Authored-By: Leon Kiefer <leon.k97@gmx.de>
@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

if you also remove line 104 ledColorBlink[i] = leds[i]; it should work, the ledColorBlink is only modified when you enable the blink and you set the blink color. You don't have to save the blink color in the animation loop, because it's (a) black or (b) the color from ledColorBlink

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

I think it will only work when blink=true. For not blinking led it is not working because the color is not set to leds array.

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Here are some test results with current code:
used topic
home/OpenMQTTGateway/commands/MQTTtoFASTLED/setled
payload

{
"led":0,
"hex":"ffffff",
"blink":false
}

-> no led lights

{
"led":0,
"hex":"ffffff",
"blink":true
}

-> led 0 starts blinking

{
"led":0,
"hex":"ffffff",
"blink":false
}

=> two different result, depends on current state of step
-> led 0 off
-> lef 0 on

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

After the tests I roll back 46b21df. I also update the topic for animation. Now it is MQTTtoFASTLED/setanimation and payload fire. All other payloads will disable the animation.

@1technophile

This comment has been minimized.

Copy link
Owner

commented Aug 1, 2019

Thanks for all this work ! Now you are close to the end. We have 2 possibilities :

  • you do a rebase to have a single commit (i can explain you)
  • i do it for you
@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I don't know what to do. I will do it when you will explain me what I should do.

@Legion2

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

@1technophile GitHub can do it for you, it's a merge option

@Legion2 Legion2 dismissed their stale review Aug 1, 2019

outdated

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Do I have anything to do yet?

@1technophile 1technophile added this to the V0.9.2 milestone Aug 2, 2019

@1technophile

This comment has been minimized.

Copy link
Owner

commented Aug 2, 2019

Yes. I can do it. If @BigDi wants to learn how to gather several commits on one he can take a look to the rebase command.

@1technophile 1technophile merged commit 0a9e8ea into 1technophile:development Aug 2, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@1technophile

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

Hello,

do you think it would be possible to write 2 wiki pages for fastled:
FASTLED hardware setup
FASTLED user guide

@BigDi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Sorry for late reply. I will do but will take 1-2 weeks because I'm really busy at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.