Strobe fix #102
No reviewers
Labels
No labels
Core
Fixed
Godot
HardwareIssue
LEDS
Networking
Problem
SoftwareIssue
duplicate
enhancement
good first issue
help wanted
invalid
on hold
question
reminder
todo 🗒️
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
squatnet/ArduinoStuff!102
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "StrobeFix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
add a timeArray[NumStrips+1]
Is this actually neccesary
@ -472,0 +587,4 @@}else if (subs.startsWith("Pul")) {DPRINT("Pulse ");doPulse();Why is
string.indexOf(',')wrapped in brackets?@ -632,2 +650,3 @@void theLights() { // speckles and strobesfadeToBlackBy(&(leds[LEDStart]), NoLEDs, 10);int myDelay = map(variableDelay, 0, 255, (standardDelay / 8), (standardDelay * 8));int myFadeOut = map(variableDelay, 0, 255, (standardDelay * 8), (standardDelay / 8));more of these pointless extra brackets
@ -739,3 +886,3 @@}}else{else {why are we doing this inside each pattern... We should simply have each pattern set its delay to an array based on its strip number and then sum the contents of the array and divide by numStrips
we should be doing something more like
if (individualStripMode == 1)....timeArr[currentStrip] = whateverelse...currDelay = whatever@ -0,0 +1,524 @@// Varidaic Debug MacroIf you are going to do this please commit and push straight away so we can compare any changes that were then made against your base even if its identical to another file
sum the array and divide by NumStrips here, if we are in indidualStripMode otherwise use the "currentDelay"
Honestly think this will be a better way of doing it. we dont wat to be calling
FastLED.delay(x)twice.Probably a currentDelay also
#100
@ -472,0 +587,4 @@}else if (subs.startsWith("Pul")) {DPRINT("Pulse ");doPulse();ahh that was legacy when i was trying to fix that parser bug, dumb idea (not that it breaks anything just unnecessary syntax) i missed removing it.
possibly not
@ -632,2 +650,3 @@void theLights() { // speckles and strobesfadeToBlackBy(&(leds[LEDStart]), NoLEDs, 10);int myDelay = map(variableDelay, 0, 255, (standardDelay / 8), (standardDelay * 8));int myFadeOut = map(variableDelay, 0, 255, (standardDelay * 8), (standardDelay / 8));as above
@ -0,0 +1,524 @@// Varidaic Debug Macrowhat you talking about? i did commit and push straight away, you have lost me totally here?
@ -739,3 +886,3 @@}}else{else {ok sounds like a neater way of doing it, everything can always be improved, as i said the other day the refactor never ends eh ;)
@ -472,0 +587,4 @@}else if (subs.startsWith("Pul")) {DPRINT("Pulse ");doPulse();Fair enough.
Will test and see if we can get those bytes back then.
@ -0,0 +1,524 @@// Varidaic Debug MacroWhat I'd normally do is, save it to both branches. The base copy to the old branch and the edited version to the new. That way a comparison between both branches could be done. It's not important if its throwaway test code really.
@ -739,3 +886,3 @@}}else{else {Aye. Just sure I've seen somewhere that having fastleds delay called twice can cause undefined behaviour. Best to just call it a single time
@ -0,0 +1,524 @@// Varidaic Debug Macroahh gotcha, means not pushing to base then?
in this instance you could always do a pull against DV2Work and get a better difference report.
@ -739,3 +886,3 @@}}else{else {yeah the more i think on it, the more i am wanting to sack this individual strip mode idea off, it certainly seems to be causing lots of complication as far as additional features are concerned.
Dont sack it off yet as I beleive it has the potential to be very useful
for what I want to do with the signpost and rubrics cube idea. If all else
fails we can just prevent strobe from being used in ind strip mode
On Fri, 7 Jun 2019, 10:36 kieran-boyle, notifications@github.com wrote:
Aye fair enough.
Solution then is to sack of the divide by four idea in each pattern. Then in loop if (individual StripMode==1) standard delay
In each pattern wrap delay with if individualStripMode == 0
I remember now why I was originally doing it that way lol.
Would mean no timing changes when individual strip mode is on. Could do some math on the standard delay instead but some patterns may look weird. (Strobe is the biggest culprit here)
Sweet. However you want to do it.
as simple as possible, as little time as possible to implement is my
thinking.
On Fri, 7 Jun 2019, 10:59 kieran-boyle, notifications@github.com wrote:
So for now, strobe can be done as described in my last post with a single
colour whilst in individual stripmode.
Add colour improvements improvements to feature list, i'll take a real look
sometime.
On Fri, Jun 7, 2019 at 10:59 AM kieran-boyle notifications@github.com
wrote:
Dude it is where to address the strobe that is causing the confusion, and without the board to be able to test I am unable to continue, as soon as you are back I should be able to crack it. Change to a different strobe unfortunately doesn't address the issue
You tried Rios suggestion of attempting to upload with the reset button
held down?
On Fri, 7 Jun 2019, 12:44 kieran-boyle, notifications@github.com wrote:
Nah not yet I'll give that a crack in a tick mate.
And just to be clear I am talking about which lines in the strobe code that need to be changed to LEDStart NoLEDs too address the correct section of the array.
I'll report back as to if the fix works in five or so.
Aye if you can get it uploading again that's sweet. I'm not really able to
read thru the code right now as supposed to be busy at work but I will find
the lines you need asap after i finish. I saw them yesterday sure of it.
On Fri, 7 Jun 2019, 12:49 kieran-boyle, notifications@github.com wrote:
Yeah that didn't work.
I thought I had it figured too, but as I can't test it kinda scuppers my efforts. Pain in there arse as it's so nearly complete ROFL
these lines look interesting
Aye, where do startpos/lastpos recieve their values from?
Get Outlook for Androidhttps://aka.ms/ghei36
From: Squatnet notifications@github.com
Sent: Friday, June 7, 2019 7:30:13 PM
To: Squatnet/ArduinoStuff
Cc: kieran-boyle; Comment
Subject: Re: [Squatnet/ArduinoStuff] Strobe fix (#102)
these lines look interesting
static void strobeDraw(uint8_t startpos, uint16_t lastpos, uint8_t period, uint8_t width, uint8_t huestart, uint8_t huedelta, uint8_t saturation, uint8_t value) {
uint8_t hue = huestart;
for ( uint16_t i = startpos; i <= lastpos; i += period) {
CRGB color = CHSV( hue, saturation, value);
//CRGB color = CRGB::Blue; // USE TO COMPLETELY BYPASS HSV Change Scheme
uint16_t pos = i;
for ( uint8_t w = 0; w < width; w++) {
leds[pos] = ColorFromPalette(currentPalette, colorIndex);
pos++;
if (pos >= NUM_LEDS) {
break;
}
}
hue += huedelta;
}
}
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com/Squatnet/ArduinoStuff/pull/102?email_source=notifications&email_token=AG3GL2R4BYJAV46FQIO5T63PZKSLLA5CNFSM4HVH4ECKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGUKLQ#issuecomment-499991854, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AG3GL2VJJYZVVYQTJHCWXGDPZKSLLANCNFSM4HVH4ECA.
Looks like this is ledStart @kieran-boyle
Heres your ledEnd @kieran-boyle
Found ledStart and end in code for strobe
sStartPosition ? aye i thought as much
aye, its odd, i was working with these two when it fragged, probably just a coincidence.
as i said earlier i am sure i had this running on individual strip mode, code seems to of been lost in the shuffle and its a bit of a confusing block ;)
¯_(ツ)_/¯
well i dunno but thats what i recon.
i just read through the code and these bits look right
yeah seems logical, im sure it wont take much to fix
You might have already figured this out but:
All period, speed and length of strobe light width is determined in the strobeDraw and sent to core.
sStartPosition is determined in the strobeCore, where the sum of "
sStartPosition = sStartPosition + dashmotionspeed;
if( sStartPosition >= dashperiod ) { if ( sStartPosition >= dashperiod ) {
while( sStartPosition >= dashperiod) { sStartPosition -= dashperiod; } while ( sStartPosition >= dashperiod) {
sStartPosition -= dashperiod;
}
" is sent to the strobeWorker. A simple iteration up and down between a small range.
In strobeDraw, the "period" is determined by the "dashwidth" from Core.
for ( uint16_t i = startpos; i <= lastpos; i += period)
This iteration will draw like:
// starting from 'startpos', up to and including 'lastpos'
// with a given 'period' and 'width'
// ** starting from a given hue, which changes for each successive dash by a 'huedelta'
// ** at a given saturation and value. -- Which is now your palette and colorIndex
//
// period = 5, width = 2 would be _ _ _ X X _ _ _ Y Y _ _ _ Z Z _ _ _ A A _ _ _
The only part of this code i've never really gotten is the relation of "w" iteration before the calling of leds[pos].
If anyone works out that mystery, let me know.
@ -1075,2 +1065,4 @@FL(1, NUM_STRIPS + 1) {patternStore[i] = (2 + i); //sets an initial value for pattern to use in indivdual strip mode.}turnOff();placing strobe back into initial pattern number.
uses standard delay if not in individual strip mode.
@ -54,3 +39,2 @@// all of this needs to be int i recon dont know why it isntint timeSinceBt = 0; //legacy currently unused. (NO IT IS NOT) set to 0 when message comes in, then increments each second (time since last message recieved.)??int timeSinceBt = 0; //legacy currently unused. set to 0 when message comes in, then increments each second (time since last message recieved.)??int autoMode = 1;//if 1 increments the pattern and palette. if 2 only increments palette.typo fix
@ -644,1 +664,4 @@}if (individualStripMode == 1) {delayStore[stripNumber] = myDelay;}wrapped each myDelay, sill think about how to address this in an array at some point. will require extra memory usage, but can see how it would look neater.
@ -747,1 +894,4 @@}if (individualStripMode == 1) {delayStore[stripNumber] = myDelay;}think i found all the addressing variables, still need to play with this pattern more, so "simple" eh :P
Strobe still isnt working. Makes the MCUS crash
Changed this variable name to avoid confusion with BRIGHTNESS
timeArrayis in now but has been calleddelayStore@ -672,3 +711,2 @@colorIndex++;if(colorIndex>254){colorIndex=0;if (colorIndex > 254) {Removed un-needed test / trigger
@ -747,1 +894,4 @@}if (individualStripMode == 1) {delayStore[stripNumber] = myDelay;}changed to work with delayStore
If we are in iStrip mode, sum the contents of the delayStore and divide by 4, cast that as an int.
Will need to fiddle when I have an mcu to do so with. I don't know why that would be the case, I can't see anything obvious that I have missed.
Think i'm just going to rebuild it. "Simple" it is not....
Basically i'm thinking just leave it commented out, Move on.
If everything else works except with the strobe. I say we just get rid of it
Aye. Glad you understand my confusion now ;) I agree leave it commented out for now, I will continue to work on it. Thing is is fucking cool when it works, so if I can get it to behave it would be preferable. Should be able to put together an actually simple strobe from scratch without to much bother though.
Aye, the damn function completes mate. I just added a whole bunch of serial statement. it runs and completes... If it is run though it either crashes in loop or in the next pattern called. Mental.
Gonna fix conflicts and merge up
fixed by removal
Moving forward
I think that's the best course of action when theres a bit of time spare..
build a new one from the ground up incorporating everything we know and
designed to run in any mode. A strobe built for the code rather than code
modified to suit the strobe.
Will discuss more later
On Sun, 9 Jun 2019, 19:55 kieran-boyle, notifications@github.com wrote:
Aye. I think I mostly have a handle on how that strobe is working now, mostly.
It only took them minutes to fix when I had a working MCU.
I think we need to have another shot on the delay function, as multiple delays for multiple strips doesn't work as intended. This can be kept in the back burner for DV3 however.
Think there may be a solution using millis timers. Will have to have a think and play with it when time allows. I can't think of a solution using fastLED.delay.
FastLED delay is quite important as it doesnt block the thread. Any other
method of delay is going to add yet more complexity and yet more headaches.
At this late stage in DV2 let refrain from reinventing the wheel again.
We've done enough of that.
We need to start being pretty brutal. It works or it's being removed.
Simple as that. We still have the 8x8 matrices and the sign boards to do.
We dont have all that much time.
On Tue, 11 Jun 2019, 09:09 kieran-boyle, notifications@github.com wrote:
of course, i am not suggesting working on it now, as stated above back burner for dv3.
i think removing strobe from individual stripmode will do the trick nicely.
alternatively removing ability to adjust timing wouldn't be a major problem.
We dont have the ability to adjust it anyway as there is no way of doing it
in the app at this moment. I recon we leave variable delay til DV3 as it
requires a lot of thought to make it work really well.
On Tue, 11 Jun 2019, 11:27 kieran-boyle, notifications@github.com wrote:
yeah not a problem, i can strip that easily enough.
i would say then, solution would be to
then everything is strong and stable.
That's what I think best saves any regressive bugs or undefined behaviour
when we are in a field later in the year. We will revisit both for DV3 as
an adjustable delay slider would look cool as hell on the app and that
strobe (when operating correctly) is blindingly awesome (literally)
With DV3 we will have a lot more information about what's going on
available to us and much closer relationship between the arduino side of
things and everything else. Devices being more aware of each other and
communication between devices without masters involvement (think devices
setting neighbours delays and such)
Til then it's not necessary
On Tue, 11 Jun 2019, 11:33 kieran-boyle, notifications@github.com wrote:
what i am saying is that i tested it thoroughly, it works great mate.
barely throughly, and it worked. no crash. woohaa.
drop it for now, bring it to table later. avanti
On Tue, Jun 11, 2019 at 12:04 PM kieran-boyle notifications@github.com
wrote:
Aye. This is closed and merged anyway. Set up another PR if it's the same branches.