I2CToFastLEDRefactor branch compare #97

Merged
kieran-boyle merged 3 commits from FastLEDKikiCheck into FastLedIStripFix 2019-06-09 20:16:17 +01:00
kieran-boyle commented 2019-05-29 20:28:36 +01:00 (Migrated from github.com)
No description provided.
kieran-boyle (Migrated from github.com) reviewed 2019-05-29 20:35:37 +01:00
@ -729,0 +858,4 @@
downReversed = 0;
}
if (posDown == 0) {
downReversed = 1;
kieran-boyle (Migrated from github.com) commented 2019-05-29 20:35:37 +01:00

localised version of gHue as not required by other patterns

localised version of gHue as not required by other patterns
kieran-boyle (Migrated from github.com) reviewed 2019-05-29 20:58:57 +01:00
kieran-boyle (Migrated from github.com) commented 2019-05-29 20:58:57 +01:00

when palette mode 1, stores the number of the palette to use, this iterates each LED and cycles 0-256 see lines 697-703,

when palette mode 1, stores the number of the palette to use, this iterates each LED and cycles 0-256 see lines 697-703,
Squatnet (Migrated from github.com) reviewed 2019-05-29 21:01:00 +01:00
@ -729,0 +858,4 @@
downReversed = 0;
}
if (posDown == 0) {
downReversed = 1;
Squatnet (Migrated from github.com) commented 2019-05-29 21:01:00 +01:00

cool, seems fine to me

cool, seems fine to me
Squatnet (Migrated from github.com) reviewed 2019-05-29 21:01:09 +01:00
@ -729,0 +858,4 @@
downReversed = 0;
}
if (posDown == 0) {
downReversed = 1;
Squatnet (Migrated from github.com) commented 2019-05-29 21:01:09 +01:00
  static int gHue;//rotates through the palette.
```suggestion static int gHue;//rotates through the palette. ```
Squatnet (Migrated from github.com) reviewed 2019-05-30 09:47:10 +01:00
Squatnet (Migrated from github.com) commented 2019-05-30 09:47:10 +01:00

Ah. I remember now why gHue is global, it's being incremental in loop. Probably better to keep it like so as if its instantiated locally it will be reset each time the function is called

Ah. I remember now why gHue is global, it's being incremental in loop. Probably better to keep it like so as if its instantiated locally it will be reset each time the function is called
kieran-boyle (Migrated from github.com) reviewed 2019-05-30 10:15:02 +01:00
kieran-boyle (Migrated from github.com) commented 2019-05-30 10:15:02 +01:00

will it not as its set to static? that was the idea anyway :) just iterate it when pattern thats using it is called?

will it not as its set to static? that was the idea anyway :) just iterate it when pattern thats using it is called?
Squatnet commented 2019-05-30 10:23:51 +01:00 (Migrated from github.com)

If its local to the pattern it is destroyed each time the pattern is called
so will always be 0

On Thu, 30 May 2019, 10:15 kieran-boyle, notifications@github.com wrote:

@kieran-boyle commented on this pull request.

In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino
https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r288916840:

// FastLED's built-in rainbow generator

  • if (paletteMode==0){
  • static int gHue;//rotates through the palette.
  • static int pHue = 0;

will it not as its set to static? that was the idea anyway :) just iterate
it when pattern thats using it is called?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGS6LUAWK4PIRTHQBS3PX6LJNA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2DJAGA#discussion_r288916840,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGIIIGXMPJAUMR4CBTM54ULPX6LJNANCNFSM4HQSG6WA
.

If its local to the pattern it is destroyed each time the pattern is called so will always be 0 On Thu, 30 May 2019, 10:15 kieran-boyle, <notifications@github.com> wrote: > *@kieran-boyle* commented on this pull request. > ------------------------------ > > In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino > <https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r288916840>: > > > // FastLED's built-in rainbow generator > - if (paletteMode==0){ > + static int gHue;//rotates through the palette. > + static int pHue = 0; > > will it not as its set to static? that was the idea anyway :) just iterate > it when pattern thats using it is called? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGS6LUAWK4PIRTHQBS3PX6LJNA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2DJAGA#discussion_r288916840>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGIIIGXMPJAUMR4CBTM54ULPX6LJNANCNFSM4HQSG6WA> > . >
kieran-boyle (Migrated from github.com) reviewed 2019-05-30 10:26:20 +01:00
kieran-boyle (Migrated from github.com) commented 2019-05-30 10:26:20 +01:00

image
am i missing something here?

![image](https://user-images.githubusercontent.com/28730858/58623198-5784cd00-82c5-11e9-9fff-e37cf5b6168f.png) am i missing something here?
Squatnet commented 2019-05-30 10:32:45 +01:00 (Migrated from github.com)

Duh no it's me I completely missed the static bit. Its early. Does that
mean it can be incremented from outside of its functions. IE gHue++ is in
loop.
I'll be honest I haven't really bothered with static yet as didnt really
see the point outside of a class. Theres no memory saving having it local
or global if you are keeping it alive with static. So yeah seems fine to me
as long as we dont decide to use gHue in another pattern in future I guess.

On Thu, 30 May 2019, 10:26 kieran-boyle, notifications@github.com wrote:

@kieran-boyle commented on this pull request.

In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino
https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r288920439:

// FastLED's built-in rainbow generator

  • if (paletteMode==0){
  • static int gHue;//rotates through the palette.
  • static int pHue = 0;

[image: image]
https://user-images.githubusercontent.com/28730858/58623198-5784cd00-82c5-11e9-9fff-e37cf5b6168f.png
am i missing something here?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGQAY55OHOJPBENC4J3PX6MTZA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2DKEQA#discussion_r288920439,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGIIIGR7U6JGCLGQB3GU5ITPX6MTZANCNFSM4HQSG6WA
.

Duh no it's me I completely missed the static bit. Its early. Does that mean it can be incremented from outside of its functions. IE gHue++ is in loop. I'll be honest I haven't really bothered with static yet as didnt really see the point outside of a class. Theres no memory saving having it local or global if you are keeping it alive with static. So yeah seems fine to me as long as we dont decide to use gHue in another pattern in future I guess. On Thu, 30 May 2019, 10:26 kieran-boyle, <notifications@github.com> wrote: > *@kieran-boyle* commented on this pull request. > ------------------------------ > > In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino > <https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r288920439>: > > > // FastLED's built-in rainbow generator > - if (paletteMode==0){ > + static int gHue;//rotates through the palette. > + static int pHue = 0; > > [image: image] > <https://user-images.githubusercontent.com/28730858/58623198-5784cd00-82c5-11e9-9fff-e37cf5b6168f.png> > am i missing something here? > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGQAY55OHOJPBENC4J3PX6MTZA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2DKEQA#discussion_r288920439>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGIIIGR7U6JGCLGQB3GU5ITPX6MTZANCNFSM4HQSG6WA> > . >
kieran-boyle commented 2019-05-30 10:43:12 +01:00 (Migrated from github.com)

No worries. It should only be incremented inside of the function. Idea was to pull as much or if loop as possible and have all variables be self contained to the functions that require them (where possible at least) just trying to build up good practice. If course

No worries. It should only be incremented inside of the function. Idea was to pull as much or if loop as possible and have all variables be self contained to the functions that require them (where possible at least) just trying to build up good practice. If course
kieran-boyle commented 2019-05-30 10:44:41 +01:00 (Migrated from github.com)

Lol sorry hit wrong button with fat thumbs. As I was saying, of course if multiple functions need a variable we are better of keeping it global, but I don't think it will be the case with this one.

Lol sorry hit wrong button with fat thumbs. As I was saying, of course if multiple functions need a variable we are better of keeping it global, but I don't think it will be the case with this one.
Squatnet commented 2019-05-30 10:49:21 +01:00 (Migrated from github.com)

Aye fair enough.

On Thu, 30 May 2019, 10:44 kieran-boyle, notifications@github.com wrote:

Lol sorry hit wrong button with fat thumbs. As I was saying, of course if
multiple functions need a variable we are better of keeping it global, but
I don't think it will be the case with this one.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGTTJMH6QRZLXH3N74TPX6OYTA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWR4HZQ#issuecomment-497271782,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGIIIGUXLG5O3PDID3LK35LPX6OYTANCNFSM4HQSG6WA
.

Aye fair enough. On Thu, 30 May 2019, 10:44 kieran-boyle, <notifications@github.com> wrote: > Lol sorry hit wrong button with fat thumbs. As I was saying, of course if > multiple functions need a variable we are better of keeping it global, but > I don't think it will be the case with this one. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AGIIIGTTJMH6QRZLXH3N74TPX6OYTA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWR4HZQ#issuecomment-497271782>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGIIIGUXLG5O3PDID3LK35LPX6OYTANCNFSM4HQSG6WA> > . >
Riolaurenti (Migrated from github.com) reviewed 2019-06-03 21:07:47 +01:00
Riolaurenti (Migrated from github.com) commented 2019-06-03 21:07:47 +01:00

I don't understand why gHue would ever be static..?
It's an incrementally changing value..
am I missing something here?

I don't understand why gHue would ever be static..? It's an incrementally changing value.. am I missing something here?
Riolaurenti (Migrated from github.com) reviewed 2019-06-03 21:08:50 +01:00
Riolaurenti (Migrated from github.com) commented 2019-06-03 21:08:50 +01:00

have you tested this code?

have you tested this code?
Squatnet (Migrated from github.com) reviewed 2019-06-03 22:48:50 +01:00
Squatnet (Migrated from github.com) commented 2019-06-03 22:48:49 +01:00

Static is not like constant it just means that the variable is kept alive after the function exits. The code is untested as far as I know. Kieran seems to think it's better to do it as a static local variable as it is only ever used for fill_rainbow so it does seem pointless to have it as a global variable seeing as nothing else references it. I dont actually think it makes any difference with regards to memory or anything and personally I would have just left it as it was as it wasnt hurting anything.

Static is not like constant it just means that the variable is kept alive after the function exits. The code is untested as far as I know. Kieran seems to think it's better to do it as a static local variable as it is only ever used for fill_rainbow so it does seem pointless to have it as a global variable seeing as nothing else references it. I dont actually think it makes any difference with regards to memory or anything and personally I would have just left it as it was as it wasnt hurting anything.
kieran-boyle (Migrated from github.com) reviewed 2019-06-04 00:59:33 +01:00
kieran-boyle (Migrated from github.com) commented 2019-06-04 00:59:33 +01:00

no makes no difference as to it being global, or static local, just means it doesnt get deconstructed. as i stated above, purpose was to remove a global that only has one calling function. good code practices and all that..

no makes no difference as to it being global, or static local, just means it doesnt get deconstructed. as i stated above, purpose was to remove a global that only has one calling function. good code practices and all that..
Riolaurenti commented 2019-06-04 08:45:42 +01:00 (Migrated from github.com)

I see, It wasn't and now is..
Surely whatever replaced gHue in the other patterns is using the memory
instead.
Read the linked page, hadn't quite got it.. Still can't see how it helps
tho.

Don't push the code before you test the code x

On Mon, Jun 3, 2019 at 10:48 PM Squatnet notifications@github.com wrote:

@Squatnet commented on this pull request.

In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino
https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r290052487:

// FastLED's built-in rainbow generator

  • if (paletteMode==0){
  • static int gHue;//rotates through the palette.
  • static int pHue = 0;

Static is not like constant it just means that the variable is kept alive
after the function exits. The code is untested as far as I know. Kieran
seems to think it's better to do it as a static local variable as it is
only ever used for fill_rainbow so it does seem pointless to have it as a
global variable seeing as nothing else references it. I dont actually think
it makes any difference with regards to memory or anything and personally I
would have just left it as it was as it wasnt hurting anything.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AFPE5K7OOJKGOYBU7TXBCP3PYWGUFA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2OJJWY#discussion_r290052487,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFPE5K7UXIVVVEMMPI3TR4DPYWGUFANCNFSM4HQSG6WA
.

I see, It wasn't and now is.. Surely whatever replaced gHue in the other patterns is using the memory instead. Read the linked page, hadn't quite got it.. Still can't see how it helps tho. Don't push the code before you test the code x On Mon, Jun 3, 2019 at 10:48 PM Squatnet <notifications@github.com> wrote: > *@Squatnet* commented on this pull request. > ------------------------------ > > In 2.aNCS_LED_DV2/current/I2CtoFastLEDs/I2CtoFastLEDs.ino > <https://github.com/Squatnet/ArduinoStuff/pull/97#discussion_r290052487>: > > > // FastLED's built-in rainbow generator > - if (paletteMode==0){ > + static int gHue;//rotates through the palette. > + static int pHue = 0; > > Static is not like constant it just means that the variable is kept alive > after the function exits. The code is untested as far as I know. Kieran > seems to think it's better to do it as a static local variable as it is > only ever used for fill_rainbow so it does seem pointless to have it as a > global variable seeing as nothing else references it. I dont actually think > it makes any difference with regards to memory or anything and personally I > would have just left it as it was as it wasnt hurting anything. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <https://github.com/Squatnet/ArduinoStuff/pull/97?email_source=notifications&email_token=AFPE5K7OOJKGOYBU7TXBCP3PYWGUFA5CNFSM4HQSG6WKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2OJJWY#discussion_r290052487>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AFPE5K7UXIVVVEMMPI3TR4DPYWGUFANCNFSM4HQSG6WA> > . >
Squatnet (Migrated from github.com) reviewed 2019-06-04 17:05:40 +01:00
Squatnet (Migrated from github.com) commented 2019-06-04 17:05:40 +01:00

Resolved

Resolved
Sign in to join this conversation.
No description provided.