hal: Update the remaining user component VFDs to getter/setter - #4319
hal: Update the remaining user component VFDs to getter/setter#4319BsAtHome wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
I went through all four components and built the branch clean. I also ran the binaries with --help and exercised a minimal repro for the handle-vs-value cases below.
There are three places where a HAL handle is tested directly instead of the pin value. hal_bool_t is an opaque pointer into shared memory; after a successful hal_pin_new_bool it is always non-NULL, so if (!handle) is always false and if (handle) is always true, regardless of the pin state. These need hal_get_bool().
The rest looks like the expected mechanical migration. I agree with leaving the existing indentation alone; reindenting now would only pollute the blame.
| if (looptime > 2.0) looptime = hal_set_real(p->haldata->looptime, 2.0); | ||
| loop_timespec.tv_sec = (time_t)looptime; | ||
| loop_timespec.tv_nsec = (long)((looptime - loop_timespec.tv_sec) * 1000000000l); | ||
| if (!p->haldata->max_speed) |
There was a problem hiding this comment.
p->haldata->max_speed is a HAL handle (opaque pointer), not the pin value. It is always non-NULL after pin creation, so this test is always false and the loop always sleeps; max-speed never takes effect. Use if (!hal_get_bool(p->haldata->max_speed)).
| if (looptime > 2.0) looptime = hal_set_real(p->haldata->looptime, 2.0); | ||
| loop_timespec.tv_sec = (time_t)looptime; | ||
| loop_timespec.tv_nsec = (long)((looptime - loop_timespec.tv_sec) * 1000000000l); | ||
| if (!p->haldata->max_speed) |
There was a problem hiding this comment.
Same handle-vs-value issue as in vfdb_vfd. Use if (!hal_get_bool(p->haldata->max_speed)).
| PIN(hal_pin_new_si32(id, HAL_OUT, &(h->status), 0, "%s.status", name)); | ||
| PIN(hal_pin_new_bool(id, HAL_IN, &(h->max_speed), 0, "%s.max-speed", name)); | ||
| PIN(hal_pin_new_si32(id, HAL_OUT, &(h->errorcount), 0, "%s.error-count", name)); | ||
| PIN(hal_pin_new_real(id,HAL_OUT, &(h->upper_limit_hz), 0.0, "%s.frequency-limit", name)); |
There was a problem hiding this comment.
missing space after the comma: hal_pin_new_real(id,HAL_OUT, ....
|
|
||
| // if 1, choose ramp times as per F500/F501 | ||
| // fix for PID loops where long ramp times cause oscillation | ||
| if (haldata->acc_dec_pattern){ |
There was a problem hiding this comment.
Missed one. Use if (hal_get_bool(haldata->acc_dec_pattern)).
2 participants
These are the remaining VFD user components. They are not especially complex, but they do have many lines changed.
Unfortunately, you need to have the hardware and modbus setup to test these VFD drivers. Not something many of us have at hand. So, we need to have some eyes on the code to see if anything got missed.
Note, a lot of the content is badly indented. A few places were corrected (like parts of the hal structs), but most of it needs to be done another time. Altering too much will just confuse the blame and make tracking any potential problems hard. So, mostly left as is.