Skip to content

hal: Update component pid to getter/setter - #4335

Merged
grandixximo merged 1 commit into
LinuxCNC:masterfrom
BsAtHome:halgs_component-pid
Aug 3, 2026
Merged

hal: Update component pid to getter/setter#4335
grandixximo merged 1 commit into
LinuxCNC:masterfrom
BsAtHome:halgs_component-pid

Conversation

@BsAtHome

@BsAtHome BsAtHome commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

The final component! Probably the largest change set for one file.

There are no parameters in this component. However, there are several pins that are only allocated conditionally. Target memory for these was allocated using hal_malloc in the else-clause. This as changes to allocate fake parameters to achieve the same function and prevent potentially compromising target memory size requirements.

There is an interesting thing for pins commandvds and feedbackvds. These are made to point to commandv and feedbackv at pin creating time, which are created pins. It states in a comment "compute command and feedback derivatives to dummysigs", but it does not make sense. The original code always lets the ...vds version point to the ...v version (these are just pointers), so its intent and actual implementation may not align.

The code to export pins in export_pid():

...
    CHK(hal_pin_new_real(comp_id, HAL_IN, &(addr->commandv), 0.0, "%s.command-deriv", prefix));
    addr->commandvds = addr->commandv;

    CHK(hal_pin_new_real(comp_id, HAL_IN, &(addr->feedbackv), 0.0, "%s.feedback-deriv", prefix));
    addr->feedbackvds = addr->feedbackv;
...

The original code where the assignment takes place.

    /* compute command and feedback derivatives to dummysigs */
    if(!(pid->prev_ie && !*(pid->index_enable))) {
        *(pid->commandvds) = (command - pid->prev_cmd) * periodrecip;
        *(pid->feedbackvds) = (feedback - pid->prev_fb) * periodrecip;
    }
    /* and calculate derivative term as difference of derivatives */
    *(pid->error_d) = *(pid->commandv) - *(pid->feedbackv);

The calculations assign to pins commandvds and feedbackvds and will effectively be written to commandv and feedbackv. No idea why this was done this way...

@grandixximo

Copy link
Copy Markdown
Contributor

Oh wow, that was quick, a good sprint to the finish line ;-)

@grandixximo

Copy link
Copy Markdown
Contributor

rip-rtai its doing its thing being slow on
http://azure.archive.ubuntu.com/ubuntu
Was your github retry thing trying to fix this? It does not look like its the github release, and I am not sure if we can put a quick retry there.

@grandixximo
grandixximo merged commit b0b9ded into LinuxCNC:master Aug 3, 2026
31 of 32 checks passed
@BsAtHome

BsAtHome commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

rip-rtai its doing its thing being slow on http://azure.archive.ubuntu.com/ubuntu Was your github retry thing trying to fix this? It does not look like its the github release, and I am not sure if we can put a quick retry there.

Yes and no. The rtai CI downloads the kernel files from the NTULINUX repo. The download would get a "connection reset" aborting the build.

The next problem is that the "Install dependencies" step in all the CI is tripping on hangs while downloading packages. Then you finally timeout and fail CI. This has been happening very often lately. Almost often enough to make the whole CI experience more trouble than its worth.

@BsAtHome

BsAtHome commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

BTW, I figured it out how the commandv and commandvds differ.

The ...vds versions never change when a signal gets attached to the pin and keep pointing to the original dummy signal. The signal attach operation changes the ...v pointer, but not the ...vds pointer. That is one bit of a sneaky way to alter function.

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