Output.cpp "=" instead of "|=" and "&="

[Plüschi]

I have never understood why Output.cpp proc initOutput() (Mega part) does this "|=" and "&="

Code: Select all

      TCCR3A |= (1<<WGM31); // phase correct mode
      TCCR3A &= ~(1<<WGM30);
      TCCR3B |= (1<<WGM33);
      TCCR3B &= ~(1<<CS31); // no prescaler
      ICR3   |= 0x3FFF; // TOP to 16383;    

instead of making a clean and precise statement like this

Code: Select all

      TCCR3A = (1<<WGM31); 
      TCCR3B = (1<<WGM33) | (1<<CS30); // phase correct mode no prescaler
      ICR3   = MOTOR_RATE;        

which is simpler, less prone to errors, shorter, faster, and incredibly sexy.
Same repeats for TCCR4A a few lines down.

I would be glad to unfuck this for you ...

[QuadBow]

which is simpler, less prone to errors, shorter, faster, and incredibly sexy.

But, your proposal implements a totally different functionality...
Try do change it, but, don't be surprised if you'll got only one or two motors running.
The same register is changed at a later stage depending on the pre-compiler definitions.

[Plüschi]

I think you are wrong. I think it only messes up is if you use a 8mhz processor instead of a 16mhz one.
Tested with 8 motors. Patch is attached.

[QuadBow]

I think you are wrong.

No, I don't believe so.

Code: Select all

 
    #if (NUMBER_MOTOR > 0)
      // init 16bit timer 3
      TCCR3A |= (1<<WGM31); // phase correct mode
      TCCR3A &= ~(1<<WGM30);
      TCCR3B |= (1<<WGM33);
      TCCR3B &= ~(1<<CS31); // no prescaler
      ICR3   |= 0x3FFF; // TOP to 16383;      
      TCCR3A |= _BV(COM3C1); // connect pin 3 to timer 3 channel C
    #endif
    #if (NUMBER_MOTOR > 1)
      TCCR3A |= _BV(COM3A1); // connect pin 5 to timer 3 channel A
    #endif
...
    #if (NUMBER_MOTOR > 3)
      TCCR3A |= _BV(COM3B1); // connect pin 2 to timer 3 channel B
    #endif

Your code is as follows:

Code: Select all

      TCCR3A = (1<<WGM31); 
      TCCR3B = (1<<WGM33) | (1<<CS30); // phase correct mode no 
      TCCR3A |= _BV(COM3C1); // connect pin 3 to timer 3 channel 

Your are applying the changes only to the first instance of register declaration.
In this case there is no difference, indeed.
But, at a later stage these registers are updated when using more than one motor.
And your patch doesn't touch these sections...

[Plüschi]

Your are applying the changes only to the first instance of register declaration.
But, at a later stage these registers are updated when using more than one motor.

I set the registers to a defined prescaler, mode and top value. Then i OR in the motors. That's perfectly sound and logical. Works nicely too.

The original as is today does OR in the counters top value

Code: Select all

ICR3   |= 0x3FFF; // TOP to 16383;  

If ICR3 was not 0 this doesent work. That feels so plain and painfully wrong.

Think it over again ... what should be wrong with it?

[QuadBow]

Think it over again ... what should be wrong with it?

There is nothing wrong - neither withyour code nor with the original code - at least if you don't apply it to all sections in output.cpp.
But, why do you want to change something?
The argument, that your proposal looks more sexy, is a bit strange.

Never change a running system.

[timecop]

The argument, that your proposal looks more sexy, is a bit strange.

Never change a running system.

THIS
Is what's wrong with this project.

[Mis]

Plushi.
You're absolutly right. Most of this | and & is totally nonsense. And plenty of flash size is not an excuse for these nonsenses.

[copterrichie]

I have never understood why Output.cpp proc initOutput() (Mega part) does this "|=" and "&="

Please, just a point of clarity, this is only for the Mega and not for the AT328? In other words, this only applies to the Mega 1280/2560 boards only?

Thank you.

[Plüschi]

The 328 initialization is done by Arduino boot code for the analogWrite() function. I think the init should also be put into output.cpp for clarity, as comment or as code. The code "as is" wont work compiled without arduino boot-init, 2560, 328 and 32u4.

The 32u4 code does the same messy "|=" and "&=" thing as the mega.

I can provide the patches for all cpu's. But i wont figure this out to have it trashed. My time is precious too.