Skip to content

Conversation

@JamesGKent
Copy link

these changes mean that instead of having to modify the library headers "NUM_TLCS" macro to select how many tlc ICs are chained its declared in the call to init

example sketches are also renamed to .ino instead of the older .pde

@Grahldg
Copy link

Grahldg commented Sep 19, 2018

Hi @JamesGKent ,

Wow! You put a lot of work into this, thanks! It is better object oriented design to have the number of TLCs in the constructor. I like that you provided a default value of 1 allowing existing constructors in our examples to work without modification. One thing I did notice is that some of the documentation is not complete. For example the constructor lacks an explanation of the additional parameter:

 \param initialValue = 0, optional parameter specifing the inital startup
           value */
void Tlc5940::init(uint8_t num_tlcs, uint16_t initialValue)

I will go through and merge any commits that are independent (changing .pde to .ino for example) and will let you know if I see any other changes that need to me made.

Thanks again!

@Grahldg
Copy link

Grahldg commented Sep 19, 2018

CircularLightBuffer.ino is not compiling due to the following error:

C:\Users\sam.wizer\Documents\Arduino\libraries\SparkFun_TLC5940_Arduino_Library\src/tlc_shifts.h:44:38: error: 'tlc_GSData' was not declared in this scope

I see that you commented out the declaration of tlc_GSData in SparkFun_Tlc5940.h which depended on NUM_TLCS and created a private pointer within the class, but this is creating compilation errors. If you can add a commit that addresses these issues I will take another look.

@JamesGKent
Copy link
Author

the headache here is backwards compatibility, in order to be able to define the number of ICs in the constructor a number of variables are moved from globals in the header to class members, this would include the grayscale data, however the shift functions are not class functions and as such have no reference to the grayscale data.

I cannot see a way to fix this without a re-architecture, which will break backwards compatibility. I could of course do this and fix the examples to suit, and the argument could be made that few people will be using that section of the library and as such there should be a minimum of people affected, are you happy to break backwards compatibility as long as the library remains feature complete?

@Grahldg
Copy link

Grahldg commented Sep 28, 2018

I'm hesitant to push a new version without backwards compatibility, but could potentially do it as an optional update. I will dig a little deeper and see if I can find a better solution.

@JamesGKent
Copy link
Author

actually i'm being daft, because the Tlc class is instantiated in the library rather than the sketch it should be possible to maintain backwards compatibility, i'm thinking that the shift functions should become class members, but the old unattached functions simply call the class members.

I'll look into this further.

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.

2 participants