machinekit / machinekit-hal

Universal framework for machine control based on Hardware Abstraction Layer principle
https://www.machinekit.io
Other
108 stars 62 forks source link

Delete HAL Param API #260

Open cerna opened 4 years ago

cerna commented 4 years ago

Hello, for some time now the HAL params were considered obsolete and given they are not so different from pins, use of pins in place of params is preferable. (Users were actually told off for use of params: machinekit/machinekit#1435.)

So in terms of spring cleaning and having leaner code-base and deleting old clutter which causes head-aches for newcomers, I propose to completely delete and purge the params API from Machinekit-HAL repository.

Opposition?

luminize commented 4 years ago

personally I would not remove it since it does not hurt. If you remove it then you will break api heavily for components and existing configurations. is that worth it?

cerna commented 4 years ago

@luminize, the only reason for it is to make Machinekit-HAL code more streamlined and simpler to understand to the new programmers. So theirs code is not rejected even when by C4 rules it technically should be merged.

I would of course solve the issues in in-repository components.

(So far, take it as a shout of fancy.)

luminize commented 4 years ago

the only reason for it is to make Machinekit-HAL code more streamlined and simpler to understand to the new programmers.

Personally, I do not think this specific case will make it easier to understand or use HAL or have developers flock to machinekit.

Iirc parameters are not widely used apart from the odd parameter somewhere. Most Paramus have been converted to pins already.

The cython api has no support for parameters already.

I’m not against no parameters :)

So theirs code is not rejected even when by C4 rules it technically should be merged.

I don’t understand this remark I’m afraid

cdsteinkuehler commented 4 years ago

Removing support for parameters would make it far more challenging to integrate a current build of LCNC on top of Machine-kit HAL, since LCNC has not deprecated parameters AFAIK.

On 3/6/2020 11:38 AM, Bas de Bruijn wrote:

the only reason for it is to make Machinekit-HAL code more streamlined and simpler to understand to the new programmers.

Personally, I do not think this specific case will make it easier to understand or use HAL or have developers flock to machinekit.

Iirc parameters are not widely used apart from the odd parameter somewhere. Most Paramus have been converted to pins already.

The cython api has no support for parameters already.

I’m not against no parameters :)

So theirs code is not rejected even when by C4 rules it technically should be merged.

I don’t understand this remark I’m afraid

-- Charles Steinkuehler charles@steinkuehler.net

luminize commented 4 years ago

On 6 Mar 2020, at 21:06, cdsteinkuehler wrote:

Removing support for parameters would make it far more challenging to integrate a current build of LCNC on top of Machine-kit HAL, since LCNC has not deprecated parameters AFAIK.

Good point!

cerna commented 4 years ago

OK, no touching the parameters API, got it.

I don’t understand this remark I’m afraid

I meant the original pull request machinekit/machinekit#1435 from @rubienr which was left in limbo for over year even through all tests passed, so theoretically it should have been merged.

Thinking about it, maybe add code-smell test.

Removing support for parameters would make it far more challenging to integrate a current build of LCNC on top of Machine-kit HAL, since LCNC has not deprecated parameters AFAIK.

They were bitten by it: Today IRC

But to the point: A) not sure it will ever happen B) not sure the positives will outweigh the negatives as this will limit the RTAPI/HAL changes a lot more and people who want LinuxCNC will just get the real thing. ->B.1) I have been thinking if the LinuxCNC with motion could not run soft real-time/in-band in Dockerized jail and connected to Machinekit-HAL over shared memory segment and let the hard real-time run in the Machinekit-HAL ->B.2) Are there any notes on this subject?

luminize commented 4 years ago

On 7 Mar 2020, at 00:11, cerna wrote: I meant the original pull request machinekit/machinekit#1435 from @rubienr which was left in limbo for over year even through all tests passed, so theoretically it should have been merged.

Thinking about it, maybe add code-smell test.

Then merge it, let’s accept the parameters. That parameter only isn’t usable with cython. B) not sure the positives will outweigh the negatives as this will limit the RTAPI/HAL changes a lot more and people who want LinuxCNC will just get the real thing. ->B.1) I have been thinking if the LinuxCNC with motion could not run soft real-time/in-band in Dockerized jail and connected to Machinekit-HAL over shared memory segment and let the hard real-time run in the Machinekit-HAL ->B.2) Are there any notes on this subject?

I’m of no help here. I claim ignorance on this :) .

luminize commented 4 years ago

@cdsteinkuehler After some thinking i came to the conclusion that I expected that with all the changes in Machinekit HAL that LCNC on top of HAL would use the components already in Machinekit HAL. If that is true then would it be blocking LCNC on top of Machinekit HAL very much?

Would it not make sense that the odd parameter could be converted to pin?

It is the question if the above will ever happen (imo), and if that is then a proof of concept for a few guys, or if that would become a wildly adopted thing being used across the machinekit and linuxcnc communities...

So if keeping open that possibility is the only thing blocking removing params, shouldn't we be open to removing params, since nobody is actively working, or contemplating on working towards LCNC on top of HAL?

cdsteinkuehler commented 4 years ago

LCNC includes at least some HAL components (eg: motion), I'm not sure if parameters are used much there or not. I do think dropping support for parameters would break a lot of legacy LCNC config files, but if we migrate to a hybrid LCNC/Machinekit-HAL there are a lot of other things I would expect to break as well.

I'm not going to stand in the way of anyone doing actual work, I was just trying to point out that dropping a feature that might ease potential integration with legacy LCNC for no better reason than "spring cleaning" might not be the wisest use of development effort...

...but since this is an all volunteer crew, and particularly since I haven't been volunteering much lately, "vote with your pull requests"!

:)

On 3/8/2020 3:54 PM, Bas de Bruijn wrote:

@cdsteinkuehler After some thinking i came to the conclusion that I expected that with all the changes in Machinekit HAL that LCNC on top of HAL would use the components already in Machinekit HAL. If that is true then would it be blocking LCNC on top of Machinekit HAL very much?

Would it not make sense that the odd parameter could be converted to pin?

It is the question if the above will ever happen (imo), and if that is then a proof of concept for a few guys, or if that would become a wildly adopted thing being used across the machinekit and linuxcnc communities...

So if keeping open that possibility is the only thing blocking removing params, shouldn't we be open to removing params, since nobody is actively working, or contemplating on working towards LCNC on top of HAL?

-- Charles Steinkuehler charles@steinkuehler.net

luminize commented 4 years ago

Thanks @cdsteinkuehler i’m of the same opinions. I do not want to stand in the way. Your remarks are very welcome. I personally do not always know the deeper layers, or why something is the way it is. So @cerna it’s your call!

zultron commented 4 years ago

Removing support for parameters would make it far more challenging to integrate a current build of LCNC on top of Machine-kit HAL, since LCNC has not deprecated parameters AFAIK.

IMO, this is important because it's the only long-term scheme to maintain a MK CNC stack that can keep up with the actively-maintained LCNC EMC application. My own (mothballed) LCNC-EMC on MK-HAL effort was working 98% last summer, and serves as proof of concept that this is technically possible, as well as doing most of the work.

What are the semantic differences between params and pins, other than that params aren't actually instantiated as pins, and what are the differences in the interfaces? LCNC HAL config files use setp for both pins and params; is there some other difference that would break existing .hal files? @luminize points out the Python API doesn't support params. If the difference is only in the current parameter C API, would it be possible to simply replace that with a compatibility API that instantiates params as pins under the covers? I'm highly in favor of streamlining code where possible for all the usual reasons, especially if it's easy to preserve compatibility.

cerna commented 4 years ago

(...) If the difference is only in the current parameter C API, would it be possible to simply replace that with a compatibility API that instantiates params as pins under the covers?(...)

I did not investigate this angle. It may pretty well be possible to wrap all call which would create HAL parameters so that they actually create PINs. There should be no naming issues given the fact that the setp call has to work based on a string identification. So if there are conflicts, these are bugs one way or another. I will have to look into this.

On the other hand, there is for example this code segment, from which I take it that the parameters were removed half-way. I just don't think that this is a good practise.

LCNC includes at least some HAL components (eg: motion), I'm not sure if parameters are used much there or not. I do think dropping support for parameters would break a lot of legacy LCNC config files, but if we migrate to a hybrid LCNC/Machinekit-HAL there are a lot of other things I would expect to break as well.

I don't want to trample over anybody's sandcastles (or however is the English collocation, I don't like the one with cereal). That is why I created this issue. But I do not think that legacy LinuxCNC configs should play any role in decision-making. These are now and going to be broken anyway. If they weren't, then we would have a LinuxCNC and I do think that LinuxCNC over Machinekit-HAL has to have some kind of edge why people should be using that instead of plain old LinuxCNC.

I'm not going to stand in the way of anyone doing actual work, I was just trying to point out that dropping a feature that might ease potential integration with legacy LCNC for no better reason than "spring cleaning" might not be the wisest use of development effort...

Maybe I am like that guy with hammer in hands for whom everything looks like a nail, but I don't think that there will be ever possibility to just take LinuxCNC modules and put them into Machinekit-HAL. That will crash and burn very quickly in horrifying spectacle. It will violate the multi-thread preconditions and it will also create problems with multi-flavour runtime.

I have been trying to come up with universal way how to tag components either by ELF inspection (like is already partially implemented in RTAPI) by compiling in REFLECTION information or by looking up libraries against which the shared library is linked (both of these at runtime). This is something all HAL components would have to implement, no way around it.

LinuxCNC gets around this issue by ignoring it: They de iure support RTAI, RT PREEMPT, XENOMAI 2 and LXRT. But de facto support only RTAI and RT PREEMPT, and they get around this by utilizing the KERNEL/MODULES definition and similar ugliness.

(BTW, every system so far is capable of supporting two flavors at the same time: vanilla posix and given RT flavour. I was thinking - I don't have it tested yet - that maybe some can support even tree flavours: vanilla posix, rt_preempt and maybe xenomai, but probably without meeting the hard-RT requirements on rt_preempt, just running as a SCHED_FIFO.)