Mixxx

Bug #1086999
Comment #19

Comment 19 for bug 1086999

Revision history for this message

Daniel Schürmann (daschuer) wrote on 2013-02-13:

#19

Thank you for your quick response and your detailed review!

> * WWaveformViewer::mousePressEvent -- I don't see why you remove the
> m_pScratchPosition->slotSet(0.0)? Does it matter?
This posbbibly leads to unwanted jumps like Bug #1117768.
I am not able to test this, but for me there is no case where it must be there,
so it is better to remove it.

> In the future could you keep the initializer lists in Google C++ style?
Ah, I just noticed that Eclipse supports the Google style well in this case.
No Problem.

> * there are some tabs in the patch
I will remove them

> * Instead of hard-coding the PID values, ...
If you need them, I will put them in again. Should they be still
undocumented? The things will getting complicated due to the buffer
size dependency.

> * I don't think you should make any changes to VelocityController. ..
It is still a PID controller. The changes I have made are just removing the sample
rate dependency from the control parameters.
My changes normalize the controller to 1 because the rate output was also normalized to 1
As you also noticed my changes for small buffer sizes are
reintroducing this sample rate dependency. This is exactly the old behavior,
But we should make It sample rate independent as well.
So I will fix this.

> * Why have you made the change to stop accumulating the error in
>VelocityController if the error is less than 0.1? ..
I have found this in a reference PID code and while tweaking the I part
I have notice that this will be required. Maybe there is also a overflow
protection, like in the reference design required, if we use the I part.
The current value 0.1 is just random.

> * reset()/setTarget() ..
For me it was not the question of a simple patch but of a simple code.
For me it is cleaner to have one calculating function with all relevant
data as parameter instead of dealing with member variables.

> * As we talked about in Bug #1117762, ..
I have removed the tweaks for scratch start. There should be no notable change
in the scratch start behavior due to my patch but I will make a patch for
Bug #1117762 soon.

Thank you for your quick response and your detailed review!

> * WWaveformViewer::mousePressEvent -- I don't see why you remove the
> m_pScratchPosition->slotSet(0.0)? Does it matter?
This posbbibly leads to unwanted jumps like Bug #1117768. 
I am not able to test this, but for me there is no case where it must be there, 
so it is better to remove it. 
 
> In the future could you keep the initializer lists in Google C++ style?
Ah, I just noticed that Eclipse supports the Google style well in this case. 
No Problem.

> * there are some tabs in the patch
I will remove them

> * Instead of hard-coding the PID values, ...
If you need them, I will put them in again. Should they be still 
undocumented? The things will getting complicated due to the buffer 
size dependency.

> * I don't think you should make any changes to VelocityController. ..
It is still a PID controller. The changes I have made are just removing the sample 
rate dependency from the control parameters. 
My changes normalize the controller to 1 because the rate output was also normalized to 1
As you also noticed my changes for small buffer sizes are 
reintroducing this sample rate dependency. This is exactly the old behavior, 
But we should make It sample rate independent as well. 
So I will fix this.

> * Why have you made the change to stop accumulating the error in
>VelocityController if the error is less than 0.1? .. 
I have found this in a reference PID code and while tweaking the I part 
I have notice that this will be required. Maybe there is also a overflow 
protection, like in the reference design required, if we use the I part.
The current value 0.1 is just random.

> * reset()/setTarget() ..
For me it was not the question of a simple patch but of a simple code. 
For me it is cleaner to have one calculating function with all relevant 
data as parameter instead of dealing with member variables.

> * As we talked about in Bug #1117762, ..
I have removed the tweaks for scratch start. There should be no notable change 
in the scratch start behavior due to my patch but I will make a patch for
Bug #1117762 soon.