Fix kinematic update

[guirodrigueslima]

When the .OFF of the real axis was updated, the .VAL of the kinematics did not update. This problem meant that the GUI interface did not update the values ​​correctly, causing an error during the operation of the beamline by researchers.

This change does not impact the current functioning of the getAxisStatus() function and solves the problem of displacement of real axes with kinematics. When the offset value is changed, the position value is changed in the first callback and returns to its state in the second callback, causing the motorStatusDone_ parameter to be updated. Is there another way to fix this problem?

Thanks!

[gilesknap]

@LeandroMartinsdS please can you take a look at this?
Thanks.

[LeandroMartinsdS]

Hi @guirodrigueslima,

Thank you for your suggestion.

I've observed the issue, but I haven't being able to reproduce your fix. For me the problem seems to remain.
Can you give us more details about your tests?

I think that a neat way of solving this would be indicating in the pmacAxis::setOffset that a move in the real motor has occurred, and let the pmacController::makeCSDemandsConsistent handle the update to VAL.
I'll be looking further into this and will post any findings here.

[LeandroMartinsdS]

@guirodrigueslima,

Am I right in suppose that in your case the Q7x (being x the real motor number) are being updated by a PLC in the controller?
(I should recall that before)

If this assumption is correct, it explains why I'm not seeing difference in the behaviour, even after your fix.

I'll keep investigating the best way of doing this.

[guirodrigueslima]

Hi @LeandroMartinsdS and @gilesknap, I hope you are well.

My problem is related to updating the kinematics .VAL field.

Note that when the real motor is moved, the .RBV fields are updated in real time (real and kinematic), when the movement is finished, the motorStatusDone_ parameter status returns to True, causing the kinematic .VAL fields to be updated (Gap and Offset kinematic). However, when the .OFF field of real motor is updated, this transition in the status of the motorStatusDone_ parameter does not exist and the .VAL field remains the old value.

I tried the pmacAxis::setOffset function, but it is not executed when the .OFF field is updated, this function is only called at IOC initialization. I thought about adding the motorRecOffset_ index of asynMotorController in the pmacController::writeFloat64 function and calling the pmacController::makeCSDemandsConsistent function, inside the function set the motorStatusDone_ kinematics to False. Afterwards, the motorStatusDone_ parameter will return to True in pmacCSAxis::getAxisStatus.

[guirodrigueslima]

@guirodrigueslima,

Am I right in suppose that in your case the Q7x (being x the real motor number) are being updated by a PLC in the controller? (I should recall that before)

If this assumption is correct, it explains why I'm not seeing difference in the behaviour, even after your fix.

I'll keep investigating the best way of doing this.

Yes, you are correct.

[LeandroMartinsdS]

@guirodrigueslima

The setOffset already is being called in pmacController::writeFloat64 using the PMAC_C_MotorOffset.

I got it working adding the following at the end of setOffset:

    int csNum = getAxisCSNo();
    if (csNum > 0) {
        pC_->csResetAllDemands = true;
        pC_->makeCSDemandsConsistent();
    }

[guirodrigueslima]

@guirodrigueslima

The setOffset already is being called in pmacController::writeFloat64 using the PMAC_C_MotorOffset.

I got it working adding the following at the end of setOffset:

    int csNum = getAxisCSNo();
    if (csNum > 0) {
        pC_->csResetAllDemands = true;
        pC_->makeCSDemandsConsistent();
    }

Hi @LeandroMartinsdS ,

I did the solution you suggested in pmacController::writeFloat64 using the PMAC_C_MotorOffset. I created a new function pmacCSController::updateAxis to activate the motion flag in the motorStatusDone_ parameter. This solution is better and we can use the pmacCSController::updateAxis function in the future.

I did some monitoring using "gdb", the problem may be related to asynMotorAxis.cpp, I started my investigation in asynMotorAxis::setIntegerParam and asynMotorAxis::setDoubleParam, but I still don't know how to solve the problem. It seems to me that the motor record is not prepared to update the .VAL without changing the status of the motorStatusDone_ parameter, even though the motorPosition_ parameter is constantly updated.

However, this change would affect all motors modules and it will be difficult to make a "pull request" in the official repository.

[LeandroMartinsdS]

@guirodrigueslima, it is not clear if you could achieve the behaviour you expected with your last commit.

The implementation seems fine to me, as I could see the .VAL changing. However I have found another issue
that may not have a straightforward solution.

There is an issue with the execution order of makeCSDemandsConsistent and the PLC that updates Q8x. Here are the details:

• Sometimes, the PLC is executed after makeCSDemandsConsistent.
• This causes Q7x to retain its previous position.
• If any axis in the same CS is moved after this, it results in an incorrect movement.

This issue becomes less frequent when executing the PLC in RT tasks, but it still depends on the execution frequency. In my tests:

• At 1 kHz, it's quite easy to reproduce the error.
• At 2 kHz, I wasn't able to reproduce it. However, I would not be optimistic about this rate if more PLCs are being executed.

I'm currently uncertain about how to resolve this issue.

[LeandroMartinsdS]

Substitute makeCSDemandsConsistent() for pCSControllers_[csNum]->updateAxis().

Also, the updateAxis can be removed from the `makeCSDemandsConsistent".

[LeandroMartinsdS]

Looking again into this, I realise that it can be solved just looking into the Q8x variables.

I have made suggestions in the code.
In that way, I'm being able to achieve the desired behaviour using a round-robin PLC without any undesired movement.

[guirodrigueslima]

Move the position update to after of the moving, and check if the position is different from the previous_position_

  moving_ = !cStatus.done_ || deferredMove_ || (position != previous_position_);

  // Store position to calculate direction for next poll.
  previous_position_ = position;
  previous_direction_ = direction;

This was my first approach before opening this "pull request", but there is a problem. With simulated motors, this solution works very well, because the position is static. However, with real motors there is a constant fluctuation of the Q8x position (last decimal places), causing the state of the moving_ variable to fluctuate over time.

Am I right ?

[LeandroMartinsdS]

Indeed, you're completely right, sorry for that.

Have edited my suggestions.

[guirodrigueslima]

I thought of another solution without makeCSDemandsConsistent.

In the pmacController::writeFloat64 function using PMAC_C_MotorOffset I can check if CS exists through the pCSControllers_ class and execute the pmacCSController::updateAxis function directly.

int csNum = this->getAxis(pAxis->axisNo_)->getAxisCSNo();
  if (csNum > 0) {
    csResetAllDemands = true;
    pCSControllers_[csNum]->updateAxis();
  }

[LeandroMartinsdS]

I think it's in the same direction of I was thinking with my edited suggestion.

Do you see any specific reason for looping over all of the CS, instead of calling the updateAxis just for the one that the current motor is?

[guirodrigueslima]

I think it's in the same direction of I was thinking with my edited suggestion.

Do you see any specific reason for looping over all of the CS, instead of calling the updateAxis just for the one that the current motor is?

Sorry, I didn't see your suggested fix above, the loop isn't necessary.

[guirodrigueslima]

I made all the modifications. It seems to be working very well! 😀

[guirodrigueslima]

Hi @LeandroMartinsdS,

Have you tested this fix, will the pull request be made?

[LeandroMartinsdS]

Hi @guirodrigueslima,

Yes, I have quickly tested it and everything seems to be in order.
I plan to take another look and aim to merge by next week.

[LeandroMartinsdS]

Closed in favor of #116