Adding support for 16 channel PWM expansion to libraries

[AlanM]

KRavEN, I'll try to figure it out. I was planning to add functions to RA_PWM to mimic the 6 channel ones surrounded by appropriate defines so that the 16 channel module would be familiar to people already running the 6 channel one.

The reason is mostly selfish, on my part, so that I can ask questions about my 16 channel one and get answers from people who know how the 6 channel one is supposed to work. 8) But I think it would also make the code simpler to maintain if it all looks the same.

[AlanM]

How much would it break if I set the 16 channel one to ints instead of bytes and everyone could be told to cast the outputs of the PWMSlope and PWMParabola and ReefCrest type functions to ints if they want to send them to the 16 channel PWM expansion? Then we could get 12-bit values in there.

I also haven't gotten my head around yet how the 6 channel expansion knows that it's being sent percents when pump modes are called and not raw values out of 255 or how the 6 channel one is using 8 bit numbers at all since I think the chip supports 12bit. Where should I be looking for that stuff?

[lnevo]

You could always overload the functions The only tricky thing is you'll need to cast your arguments as int.

[AlanM]

OK.

I found the 100-255-4095 mapping part in RA_PWM.cpp

Code: Select all

	
        Wire.write((int)(data*2.55));			// send the data
	Present=Wire.endTransmission();		// stop transmitting
	if (cmd<PWM_EXPANSION_CHANNELS) ExpansionChannel[cmd]=data;
	// Also send data to new module PCA9685
	int newdata=(int)(data*40.95);

[rimai]

You could always overload the functions The only tricky thing is you'll need to cast your arguments as int.

Overloading won't break anything
All you have to do is declare the same function with a different size.
For example... We already have this:
SetChannel(byte Channel, byte Value)
You can declare another one like this:
SetChannel(byte Channel, int Value)

This way, when you use the code like this:
byte a=10;
SetChannel (0,a);
It will use the first one.
And if you use like this:
int a=10;
SetChannel (0,a);
It will use the second one.

The compiler is smart enough to find out which variable size you are using and uses the correct one.

[rimai]

If you look at the RA_NokiaLCD library, you will see a bunch of overloads of the same function as a reference for you to wrap your head around the idea.

[AlanM]

Awesome. I did that for the DC pump constructor to add in my idea of a threshold so it doesn't break uses that don't include the threshold argument.

[rimai]

I went ahead and merged the request, but had to make a small change due to conflict from a pending merge from brunnels.
It was just a comment he added, so no big deal.
I also changed the default and initial threshold to 30.
Great stuff

[AlanM]

I'm pretty excited to be doing this. 8) Thanks!

I commented on the threshold of 30 thing on github, but I think it should be 0 so that the default is essentially the previous default, which was actually 0. Then current code will work the same as it always did, and if someone wants to set it to 30 to use the new feature, they can. Apparently the vortechs can run all the way down to 0? At least that was lnevo's objection when I proposed adding the threshold right to the wave function outputs.

[lnevo]

Yeah I use the library modes when setting my vortech in custom modes. The threshold should be set in the dcpump class not in the global function...roberto can you clarify?

[AlanM]

It is in DC pump not the global. I was just advocating for a default threshold of zero which was the defacto old threshold before there was a new way of calling DC pump.

[lnevo]

Ah ok. DCPump would be great to change there. The only thing is for people who have chosen very low values due to the threshold can now fix their code or leave it.

[rimai]

Yes, the threshold only applies to DCPump, so I think we should leave as 30 to ensure pumps don't go below the minimum and get stuck.

[AlanM]

Great. I didn't get that the vortech users use a different class.

[Meldrath]

Alan, where are you at on this?

[AlanM]

Will have the 16 channel functions into RA_PWM by the weekend and then will send a pull request to Roberto. The threshold thing for various DC controlled pumps is in the dev branch now and should work fine, but I have some questions about what the PWM write functions are doing.

Roberto, what is going on in RA_PWM with the code that actually writes the values down the wire? It looks like you first assume that the data is a percentage and you write it to the 8 bit dimmer ports on the relay box (if that's what is at 0x8) and then it gets written to the 12 bit dimmer ports on the 6 channel expansion at 0x40 with the same value? Why are you writing it to two different devices?

Code: Select all

void RA_PWMClass::Expansion(byte cmd, byte data)
{
	Wire.beginTransmission(I2CPWM);  // transmit to device #8, consider having this user defined possibly
	Wire.write('$');				// send the $$$
	Wire.write('$');
	Wire.write('$');
	Wire.write(cmd);				// send the command
	Wire.write((int)(data*2.55));			// send the data
	Present=Wire.endTransmission();		// stop transmitting
	if (cmd<PWM_EXPANSION_CHANNELS) ExpansionChannel[cmd]=data;
	// Also send data to new module PCA9685
	int newdata=(int)(data*40.95);
    Wire.beginTransmission(I2CPWM_PCA9685);
    Wire.write(0x8+(4*cmd));
    Wire.write(newdata&0xff);
    Wire.write(newdata>>8);
    Wire.endTransmission();
}

[rimai]

0x8 is the old dimming expansion module that was 8bit resolution. 0x40 is the new dimming expansion module. We have to write to both because we don't know which module someone would have.

[AlanM]

Ah, got it, thanks.

[AlanM]

Roberto,

Is there a way to detect which 6 port expansion the user has with read or something to see if there's something at address 0x8 or 0x40?

I'm trying to overload some of the writing functions for the regular 6-channel PWM expansion with integers as well as bytes to allow for the smoother dimming that would come from the 256 or 4096 PWM levels, but I'm having trouble with some of the functions and not knowing whether I should multiply the input by 2.55 or by 40.95 when setting a percent dimming.

[rimai]

The easiest way for you is to create two overloads.
One for 8bit and another for 12bit.
The 8bit would be like this:
SetSmoothChannel(byte channel, byte value)
And the 12bit would be like this:
SetSmoothChannel(byte channel, int value)
The compiler is smart enough to distinguish what you are using.

[rimai]

The other way is to check for presence of the module like this:

Code: Select all

Wire.beginTransmission(I2CPWM);
present = Wire.endTransmission();
if (present==0)
{
  //Do something, we got 8bit
}
else
{
  //Do something, we got 16bit
}

[AlanM]

Right, I'm doing that.

One weirdness is that the 8bit values are actually interpreted everywhere as percents, and the int values will be absolute int's from either 0 to 255 or 0 to 4095, not percents. I think I have it worked out.

I'm overloading everything between bytes and ints and trying to make the old versions that used bytes still work so no one gets broken code if they don't change anything because they assumed those were percentages, but if someone decides to use the new stuff and uses ints we interpret that as either 0-255 or 0-4095 raw values, not percents. Does that seem reasonable?

But then I run into the problem that the ExpansionChannel[] array is a byte array, so I create a new array called ExpansionChannelInts and have to start checking in the GetChannelValue function to see which one has values in it and return the right one because I can't overload the GetChannelValue function to produce both int and byte outputs. Can only overload with arguments, heh.

[AlanM]

The other way is to check for presence of the module like this:

Code: Select all

Wire.beginTransmission(I2CPWM);
present = Wire.endTransmission();
if (present==0)
{
  //Do something, we got 8bit
}
else
{
  //Do something, we got 16bit
}

Cool, I see that you do that with your Present variable now. Not sure where that var is read, but it makes sense.

[AlanM]

I'm trying hard to make it not change functionally at all if someone doesn't change their code as one constraint, but I'm also trying to get more resolution out of the PWM pins. Instead of the 100 levels of dimming which seems totally fine for pumps it would be nice to have 255 or 4096 across the board for light. Just overloading the PWM class functions for different arguments types isn't getting me there. It seems like passing in a constant from the main .ino file when making the call is by default putting in an int which the runtime casts to a byte when the function is executed, so the byte version of the overloaded function never gets called anyway. I could have totally different functions that I'd name as "HiRes" or something, but it seems silly to do that.

The pump profiles use the DCPump classes and assume you're putting in percentages from 0 to 100, and they call the PWM classes and feed in numbers from 0 to 100 to those classes to adjust the pump speeds. These are fed in explicitly as bytes because that's what the pump profiles return.

For the light profiles (PWMSlope, PWMParabola, and the one I added recently, PWMSmoothRamp) instead of overloading them I'm leaning towards making them accept arguments in percents as bytes, but output 12-bit integers from 0 to 4096. Then I will know that if a channel is getting a "byte" as an argument it's coming from a pump and it should be interpreted as a percent but if a channel gets an "int" as an argument it's coming from a light profile and the 12-bit int can be scaled down to 8-bit for the Actinic, Daylight, LowATO, or old 6-channel PWM expansion before writing or left alone and just written if it's on the newer 6-channel PWM expansion or 16-channel expansion.

That way everyone gets more levels of dimming for their lights and uses the max capability of whatever chip they have, and they don't have to know that the internals were changed to pass around ints as 12-bit PWM levels rather than percents as bytes. I'd leave alone the various Override functions except to change the GetChannelValue functions to multiply the Override value, which is 0-100, by 4095 and cast it as an int before returning it.

Does that seem reasonable?

[rimai]

You are right, I think the int overload will always be picked up.
But changing the arguments as byte would still not give you the desired result you need.
If you enter arguments in bytes and scale up to int, you loose the resolution you are seeking.
To make it easier for everyone, including you, I think we should just create another function. Something like SetChannelHiRes or SetChannelRaw, which would only work on the new dimming modules.

[AlanM]

I would enter arguments to the lights profiles as byte from 0 to 100 but the instant by instant calculation would be done using 12 bit ints which would get sent to the pwm pins. That's where the resolution matters, not in the input arguments.

[rimai]

Ahh. nice solution

[AlanM]

Are those PWMSlope, PWMParabola functions used as anything other than as input to the PWM write classes that you know of?

If they are exclusively inputs to PWM, maybe no one will notice if the output changes from 0-100 to 0-4096. 8)

[rimai]

I don't think anyone ever used for anything else.

[AlanM]

Thanks for all the help. Off to change stuff and try not to break your cool controller.