Are HAL names unique across object types?

[mhaberler]

HAL names of a given type (e.g. a pin, component, thread or instance) must be unique, or one gets a 'duplicate pin.. name' error.

However, it is unclear whether there is a uniqueness assumption across types. It seems - currently not, like you could have a thread called 'servo' as well:

$ halcmd -f -k
halcmd: newinst or2 servo
halcmd: newthread servo 1000000
halcmd: show inst 
Instances:
 Inst  Comp  Size  Name                      Owner
  125 32770    16  servo                     or2                 

halcmd: show thread 
Realtime Threads (flavor: posix) :
     Period  FP     Name               (     Time, Max-Time )
    1000000  NO                  servo (        0,        0 )

There is a strong case for making HAL names unique across types: it drastically simplifies HAL object management, and gets rid of literally hundreds LOC (if not more). If this were the case, would enable a huge simplification of HAL code, as lookup by name would yield one and only one HAL object.

Is there any use for non-unique HAL names?

If not, what I propose is:

• deprecate the uniqueness assumption.
• in a first step, add code to HAL to check for uniqueness of names, and output a warning from halcmd as well as the Cython bindings. This should turn up any existing configs which rely on non-unique names.
• after a a while, make non-unique names a hard error.
• now we're free to simplify the HAL object management code.

For automated testing against this proposed change, see https://github.com/machinekit/machinekit/issues/220#issuecomment-119824461

[luminize]

I'm no programming expert but non unique names sound really silly to me. I would never have thought this would be so.

It would be very easy to mess up a configuration and make it non readable.

On 09 Jul 2015, at 07:25, Michael Haberler notifications@github.com wrote:

HAL names of a given type (e.g. a pin, component, thread or instance) must be unique, or one gets a 'duplicate pin.. name' error.

However, it is unclear whether there is a uniqueness assumption across types. It seems - currently not, like you could have a thread called 'servo' as well:

$ halcmd -f -k halcmd: newinst or2 servo halcmd: newthread servo 1000000 halcmd: show inst Instances: Inst Comp Size Name Owner 125 32770 16 servo or2

halcmd: show thread Realtime Threads (flavor: posix) : Period FP Name ( Time, Max-Time ) 1000000 NO servo ( 0, 0 ) There is a strong case for making HAL names unique across types: it drastically simplifies HAL object management, and gets rid of literally hundreds LOC (if not more). If this were the case, would enable a huge simplification of HAL code, as lookup by name would yield one and only one HAL object.

Is there any use for non-unique HAL names?

If not, what I propose is:

deprecate the uniqueness assumption. in a first step, add code to HAL to check for uniqueness of names, and output a warning from halcmd as well as the Cython bindings. This should turn up any existing configs which rely on non-unique names. after a a while, make non-unique names a hard error. now we're free to simplify the HAL object management code. — Reply to this email directly or view it on GitHub.

[mhaberler]

see https://github.com/machinekit/machinekit/issues/220#issuecomment-119838889 - pretty clever!

[mhaberler]

@luminize - it is one of things which go back a decade or more and there is no rational answer for :-/

the simplification comes from here:

• have a look at say two named HAL objects, the hal_inst_t descriptor and the hal_pin_t descrptor
• notice the common fields: name, handle/inst_id, next_ptr
• however there is no common layout for this common HAL object header
• this forces per-object-type search by name/search by id/alloc/free code, which is repeated for every single HAL object, with separate active lists and separate free lists
• example: pin - find by name, find by id
• example - instance - find by name, find by id
• repeat for: comp signal param thread funct vtable ring alias

now, if descriptors had a common header with fixed layout, and a type tag, one could delete all this repeated code and replace it by a single generic findbyname/findbyid routine, as well as generic alloc/free management ( a rich source of bugs so far ;-)

[machinekoder]

In programming languages the issue of unique "names" is solved by using CamelCase, unscore_case + Hungarian notation or any other useful naming convention. There is no reason to make names not unique.

Only exception: instance/component name and function names. Usually the primary function has the same name as the component.

[mhaberler]

@strahlex on exception: the benefits to HAL core code will come only if there are no exceptions

we need to patch those up

[ArcEye]

strahlex commented 6 hours ago Only exception: instance/component name and function names. Usually the primary function has the >same name as the component.

mhaberler commented 2 hours ago @strahlex on exception: the benefits to HAL core code will come only if there are no exceptions we need to patch those up

I unintentionally 'fixed' the default function names a while back! (now re-fixed)

Would be easy to insert an 'i' say in front of all non-unique function names, at the expense of breaking all existing configs. (forget the original next bit - brain scrambled)

[ArcEye]

example: pin - find by name, find by id example - instance - find by name, find by id repeat for: comp signal param thread funct vtable ring alias

Thinking out load, is it feasible to have a common structure which covers all the different types?

There will be some wastage in each structure sure, but an advantage is that you can index through all of them with one function and just check char name[HAL_NAME_LEN + 1]; which is common to all of them.

The structure itself can have a type flag, to show what is being held and thus inform other types of searches

[mhaberler]

@ArcEye - common structure: yes, I think along these lines as well

descriptors would have a common HAL object header, which contains all the stuff any named object needs to have: type tag, name (reference), id, owner id if any, size - NB we could get rid of the fixed name size (and its waste of memory) in one go

also, the ugly habit of referencing directly fields in descriptor structs would have to end, they would have to go through static inline getter/setter functions

as have the names, and the names would not be stored in the descriptor proper but in a string table (as an offset, or some other key)

there is significant upside, but the work is going to be a bloodshed

[mhaberler]

@strahlex @ArcEye - re instance and function name

what I can imagine is:

• export the funct as instname_funct or f.instname
• modify addf to try the modified name if the 'addf instname thread' is about to fail, create a warning to fix the config and addf the modifed name anyway

[ArcEye]

I would favour the shortest option, f. or similar, prepended is probably easier than appended. Changing instcomp will be trivial. It already differentiates between default function names and actual named functions.

I'll have a look at halcmd tomorrow, to see what the easiest route is. Will need to cater for those few components with named functions too, as those do not need any addition to them, if the base name of the component is unique, so will be the function.

[mhaberler]

@ArcEye - instcomp - fine with f.instance; if outher cases come up - same procedure (first letter of type name)

the addf aliasing/warning probably needs to be done in the HAL library, so both Python + C bindings + halcmd are covered identically.

I'm currently looking into the common halobject header and how to best do this; the header isnt so much of an issue as is a solid design of the getter/setter methods

I would guess a week maybe until the worst is shaken out; would you have time + nerve to chip in ?

[ArcEye]

I am OK until later next week, when I have relatives down for a few days.

See how far we get and keep it in a branch until 'completely sorted' to keep the time pressure to minimum

[mhaberler]

@ArcEye - great, I get started at it, with the common header and accessors

let's work through my github repo branch halobject-header

btw the funct->f.funct change - would you want to get started at that in instcomp and the HAL library to match? good start at fiddling with the HAL library. AFAICT it affects hal_del_funct_from_thread and hal_add_funct_to_thread only

also, a legacy warning macro similar to HALERR would be great, so warnings could be routed a different path lateron

[machinekoder]

f.instname is very ugly in my opinion. Can't we make it instname.main,instname.task or something that makes sense from a programmers perspective? I mean all the pins are prefixed it makes no sense to do different things with the function names.

I don't understand why instname.write and instname.read are problematic. There is no aliasing with these functions.

[ArcEye]

We can do. Need to decide if the RT component option of having its own function is ever going to be used.

If it is, that will possibly further complicate and possibly inform naming

[ArcEye]

The other thing that occurs now I have looked back at instcomp

Do we want all functions identifiable as such, or are we happy just to append .funct to the component name if the function defaults to that name? eg debounce.0.funct gantry.0.read

Both are functions but read could be a pin, without knowledge of the component hard to tell

So do we use gantry.0.read.funct so that all functions will have that last section to their name?

addf can append that to all calls which lack them, without having to further differentiate between default naming and actual named functions.

That way all configs will continue to work and not only are functions differently named to the component, but can be identified as such easily

Once we get this bit clear I can crack on and implement it.

[mhaberler]

@ArcEye - re "Need to decide if the RT component option of having its own function is ever going to be used."

yes it will, it just hasnt happened yet. For instance, kinematics modules do not have a thread function but are RT; they export a vtable instead.

[mhaberler]

@ArcEye - re .funct suffix: I would think disambiguating the "function defaults to name" case is the only requirement and that covers name uniqueness

it would also have less impact / warning noise than appending ".funct" to all functions

[ArcEye]

mhaberler commented @ArcEye - re "Need to decide if the RT component option of having its own function is ever going to be used." yes it will, it just hasnt happened yet. For instance, kinematics modules do not have a thread function but are RT; they export a vtable instead.

There is a reserved commented out block in instcomp already for rt component functions Just added .rtfunct suffix to distinguish it from instance functions when that arises

################  stub to allow 'base component to have function if req later ####
#
#    print >>f, "    // exporting an extended thread function:"
#    print >>f, "    hal_export_xfunct_args_t xtf = "
#    print >>f, "        {"
#    print >>f, "        .type = FS_XTHREADFUNC,"
#    print >>f, "        .funct.x = (void *) funct,"
#    print >>f, "        .arg = \"x-instance-data\","
#    print >>f, "        .uses_fp = 0,"
#    print >>f, "        .reentrant = 0,"
#    print >>f, "        .owner_id = comp_id"
#    print >>f, "        };\n"
#
#    print >>f, "    if (hal_export_xfunctf(&xtf,\"%s.rtfunct\", compname))"
#    print >>f, "        return -1;"
##################################################################################

[ArcEye]

mhaberler commented 16 minutes ago @ArcEye - re .funct suffix: I would think disambiguating the "function defaults to name" case is the only requirement and that covers name uniqueness requirement it would also have less impact / warning noise than appending ".funct" to all functions

That is fine, just wondered given that very few components have named functions, whether clearly identifying a function by its name would be useful

[ArcEye]

PR submitted for work to date.

extract from test

mick@INTEL-i7:/usr/src/machinekit-arceye/src$ DEBUG=5 realtime restart
mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halcmd loadrt debounce
<commandline>:0: Realtime module 'debounce' loaded
mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halcmd show funct
Exported Functions:
  Comp   Inst CodeAddr  Arg       FP   Users Type    Name
 32770     72 7f67abdff076  7f67b13170e8  YES      0 xthread debounce.0.funct
 32769        7f67b13a5b2e  00000000  NO       0 user    delinst
 32769        7f67b13a59af  00000000  NO       0 user    newinst

mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halcmd newthread servo 1000000 fp
mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halcmd addf debounce.0 servo
<commandline>:0: Function 'debounce.0' added to thread 'servo'
mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halcmd show thread
Realtime Threads (flavor: rt-preempt) :
     Period  FP     Name               (     Time, Max-Time )
    1000000  YES                 servo (        0,        0 )
                  1 debounce.0.funct

mick@INTEL-i7:/usr/src/machinekit-arceye/src$ halrun -U
<commandline>:0: Realtime threads stopped
<commandline>:0: Realtime module 'debounce' unloaded

Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt export_halobjs() ip->localpincount set to 8
Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt hal_export_xfunctfv:70 HAL: exporting function 'debounce.0.funct' type 1
Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt hal_pin_new:121 HAL: creating pin 'debounce.0.funct.time'
Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt hal_param_new:135 HAL: creating parameter 'debounce.0.funct.tmax'
Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt hal_param_new:135 HAL: creating parameter 'debounce.0.funct.tmax-inc'
Jul 10 14:29:10 INTEL-i7 msgd:0: hal_lib:24293:rt instantiate - instance debounce.0 creation SUCCESSFUL

Jul 10 14:29:31 INTEL-i7 msgd:0: hal_lib:24314:user hal_xinit:246 HAL: legacy component 'halcmd24314' id=108 initialized
Jul 10 14:29:31 INTEL-i7 msgd:0: hal_lib:24314:user hal_add_funct_to_thread:254 HAL: adding function 'debounce.0' to thread 'servo'

***
Jul 10 14:29:31 INTEL-i7 msgd:0: hal_lib:24314:user hal_add_funct_to_thread:277 HAL WARNING: 'debounce.0' should be added to thread as 'debounce.0.funct'
***

Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt hal_exit:258 HAL: removing component 32770
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt free_inst_struct:339 HAL: calling custom destructor(debounce,debounce.0)
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt delete inst=debounce.0 size=400 0x7f67b13170e8
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt delete: int instance param: pincount=-1
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt rtapi_exit: freed module slot 2, was HAL_debounce
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user  'debounce' unloaded
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24341:user hal_exit:258 HAL: removing component 115
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24341:user hal_exit:258 HAL: removing component 114
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24341:user ULAPI:0 v0.1~halobject-header~94be5b1 exit
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user pid=24293 flavor=rt-preempt gcc=4.9.2 git=v0.1~halobject-header~94be5b1
Jul 10 14:30:44 INTEL-i7 msgd:0: ulapi:24365:user hal_xinit:239 HAL: hal_lib24365 initialization complete
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24365:user hal_xinit:246 HAL: legacy component 'halcmd24365' id=117 initialized
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24365:user hal_exit:258 HAL: removing component 117
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24365:user hal_exit:258 HAL: removing component 116
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24365:user ULAPI:0 v0.1~halobject-header~94be5b1 exit
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user pid=24293 flavor=rt-preempt gcc=4.9.2 git=v0.1~halobject-header~94be5b1
Jul 10 14:30:44 INTEL-i7 msgd:0: ulapi:24368:user hal_xinit:239 HAL: hal_lib24368 initialization complete
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24368:user hal_xinit:246 HAL: legacy component 'halcmd24368' id=119 initialized
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt rtapi_app_exit:206 HAL: removing RT hal_lib support
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt rt_task_delete 1 "servo:0"
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt hal_exit_threads:280 HAL: all threads exited
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt hal_exit:258 HAL: removing component 32769
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt rtapi_exit: freed module slot 1, was HAL_hal_lib
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt rtapi_app_exit:210 HAL: RT hal_lib support removed successfully
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user  'hal_lib' unloaded
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24293:rt RTAPI:0 exit
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user  'rtapi' unloaded
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app:24293:user exiting mainloop (by remote command)
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24368:user hal_exit:258 HAL: removing component 119
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24368:user hal_exit:258 HAL: removing component 118
Jul 10 14:30:44 INTEL-i7 msgd:0: hal_lib:24368:user ULAPI:0 v0.1~halobject-header~94be5b1 exit
Jul 10 14:30:44 INTEL-i7 msgd:0: rtapi_app exit detected - scheduled shutdown
Jul 10 14:30:46 INTEL-i7 msgd:0: msgd shutting down
Jul 10 14:30:46 INTEL-i7 msgd:0: zeroconf: unregistering 'Log service on INTEL-i7.local pid 24288'
Jul 10 14:30:46 INTEL-i7 msgd:0: log buffer hwm: 0% (48 msgs, 4523 bytes out of 524288)
Jul 10 14:30:46 INTEL-i7 msgd:0: normal shutdown - global segment detached

@strahlex In building found some original void returns from the old component remained in at_pid function. Amended to return 0: If you want to edit those yourself, I can ditch the changes when we come to rebase.

[mhaberler]

well they have a HAL type anyway - halcmd can tell them apart as does cython, so we need the rename only to get rid of the clash, but not for identifying the type

suggestion: rtfunct -> funct (no reason to assume RT, as posix threads are coming towards newthread ;)

[mhaberler]

oh I see, that would be an optional funct for the component without instantiation call. Hm.

[mhaberler]

I'm still working foundation stuff - when done, in all of HAL there will be a single iterator function for named objects left. Tons of code to throw out.

[ArcEye]

suggestion: rtfunct -> funct (no reason to assume RT, as posix threads are coming towards newthread ;)

The rtfunct suffix was not for uniqueness, so much as to allow humans to work out what is what.

It shows up as RT in the halcmd listing, that was the basis of the name

The base component will be the only one able to use [component].funct, an instance will always be [component].N.funct or [anothernamecompletely].funct but I thought having a different function designation would make it clearer which component it belonged to.

It is easily changed, lets see how it all shakes out

[mhaberler]

I'm progressing - a bit; it turns out more change than I thought, but all under the hood and machinekit still running, so it seems no unknown assumptions violated so far.

(below I chose vtable for a start because it is minimal impact and no legacies)

current plan:

• farm out common fields into a halhdr_t struct which is part of any named HAL object (status: defined, vtable code changed to use it)
• drop all per-object type descriptor management and work all objects through a single linked list with unique names and ID's (partial - these go out: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_priv.h#L195-L213, something like this is left - the root of the named HAL universe: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_priv.h#L193)
• replace per-object type memory management and lists by rtapi_heap (malloc/free for shm). Status: intended, but not urgent; typically there is just minor waste and that change is not critical. This will require swapping shmalloc_down and shmalloc_up (descriptors and actual RT scalars) as rtapi_heap assumes memory extension goes upwards, not downwards, but again a lowlevel detail.
• define a generic HAL object iterator to select (an) object(s), and apply operations to the result set through a callback, with a means to pass arguments/results back and forth. Status: done, might need getting used to, but proud of it ;)
• remove common fields from descriptors, and replace by accessors. Status: accessors defined, in use by vtable. No issues, just a change of habits - no more direct field fiddling. Starting to remove legacy common fields in vtable now.
• prop up hal_list.h to sport more useful functions like an iterator, and remove name collisions. This was kind of a half-baked affair to start with. Currently the halhdr_t uses a doubly-linked list which is not really needed; single-linked is just fine. But not urgent. Only entry removal needs thought and exercising, the rest is just a sub-case of the double-linked list.
• hal vs halpr functions (with or without the halmutex held). This used to permate all of the HAL API, with the general idea to export the locked functions for general use, and the unlocked ones for internal use. I found a neat way to make locking a parameter to a function, so they can be all either-or; I use the halg prefix for that (hal generic). This looks remote but its a key affair - there are doubly-defined functions already, one locked, one unlocked, which is a hack. locked looks nice but only solves trivial problems because you cannot nest locked calls - to do so, they need all to be unlocked functions and a single lock aquire/release around the whole affair. There is now a pattern to do that.

So next steps are:

• piecemeal kill common fields from the hal_xxx_t structs, and fix using code by using accessors - boring but not difficult. Once we have that, we can completely change the internal structure as to how HAL object access works, and it is going to be isolated to a few headers and one place instead of permeating dozens of source files.
• fix halcmd as it breaks. This should never have been exposed to such low-level layout details.
• fix the cython bindings as they break.

I would be interested in feedback on the halhdr_t accessor (a bit) and generic object iterator (very much) design, in particular: is it comprehensible or did I pull a xxx-name-withheld-xxx ;) ?

It will come out as the single entry point into looking up HAL objects eventually (NB it does NOT expose any storage details to using code, like list pointers, common struct fields in objects etc - meaning we are free to choose alternative low-level mechanisms should the need arrive)

these are the key locations:

• halhdr_t and accessors: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_object.h#L14-L69
• the halg_foreach iterator: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_object.h#L72-L102
• the object selectors which specialize halg_foreach for a particular task, this is just a start: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_object_selectors.h
• two combined using examples: https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_vtable.c#L174-L197

[ArcEye]

Quite a bit of work there Michael !

I would be interested in feedback on the halhdr_t accessor (a bit)

hal_object.h : 52 static inline void hh_set_invalid(halhdr_t *o) { o->_valid = 1; } cut and paste error? should be = 0

Otherwise looks quite straight forward. getters and setters are the cornerstone of OOP in C++ classes, if everyone uses them, you can do whatever you want internally without affecting any one else's code.

and generic object iterator (very much) design, in particular: is it comprehensible or did I pull a xxx-name-withheld-xxx ;) ?

Initial impression, a bit opaque. I instinctively don't like the callback functions because it makes it far from clear what is going on. It is however a very neat method, but I am going to have to look at the code for them.

The bottom line is that if all the user of the API sees is the higher level function halpr_find_vtable_by_name(const char *name, int version) for instance it does not matter how the return is arrived at, so long as the code is maintainable and consistent.

Good work, I will do a pull and try to get my head properly around it

[ArcEye]

I instinctively don't like the callback functions because it makes it far from clear what is going on.

OK, they have grown on me! Should be a method that can use a small set of callbacks, one per type of return, rather than repeated linear functions, which will become handy when replacing pages of old functions. Very neat as I said.

All builds and runs OK this end.

The runtests will of course fail, because now we have .funct tacked on the end again. Just as well you want to get rid of them.

[mhaberler]

@ArcEye - spot on on the hh_set_invalid issue, thanks!

I really appreciate the effort of reading through this semi-baked affair! I was aware that the iterator is a bit opaque but once you've worked a few examples it starts to sink in.

Meanwhile, as part of the code deduplication exercise, I have slid in the shared memory allocator (rtapi_heap.h/c) into HAL which was ready but only slightly used so far

I've rearranged the HAL library to use the rtapi_heap functions (malloc/free/realloc/calloc) instead of the legacy allocator which never freed memory, for all the descriptors. And... we're still chin above water.. this permits to do away with all the per-type freelists as memory handling including free now goes through a generic routine. Bang, another boatload of code goes out the window. The vtables alloc/free is now already on heap alloc/free and no surprises.

The only memory which is still lost (as before) on unload is the actual pin/signal/thread chain memory which goes through another allocator for cache friendliness.

I know it's a bit of creeping scope, but then that is something which had bugged me for a long time, and it really held me up on the halpipes work. I guess it's still two days or so of machete work but light on the horizon.

[ArcEye]

I've rearranged the HAL library to use the rtapi_heap functions (malloc/free/realloc/calloc) instead of the legacy allocator which never freed memory, for all the descriptors. And... we're still chin above water.. this permits to do away with all the per-type freelists as memory handling including free now goes through a generic routine. Bang, another boatload of code goes out the window. The vtables alloc/free is now already on heap alloc/free and no surprises.

That makes sense, I didn't realise that memory allocation was so convoluted.

I have got so used to using Qt, where anything allocated within a class is automatically freed by its destructor, that I forget about the years of scrupulously making sure there is a matching free for every malloc, or your precious memory disappears.

Let me know if you have a specific task you want me to look at.

I am more familiar with the halcmd level code, but a focused sub section of rtapi internals is OK

[mhaberler]

hm, one coming up (and as of yet an isolated but nasty problem) is the completion code in halcmd. It directly fumbles the (legacy) HAL lists (next_ptr etc) to generate a list matching names. It is pretty awful like the rest of halcmd - see halcmd_completer().

I need to disable that for now, but it'd be great if we have a solution eventually

I think the way to do that is a prefix-matching halg_foreach iterator which assembles the matching names into a zlist (see the code in do_unload_rt for a zlist example with a list of strings). The zlist would be passed in via user_ptr1 for instance.

want to chew on that one?

I am currently removing the old list pointers one by one, now at instance.. rebugging ;) and sort of discovering how to restructure code with the new iterators. very easy to do custom ones, and more general ones, so I might replace the per-type iters with a generic one.

[mhaberler]

the cython bindings will be super painful, even halmodule.cc :-/

[ArcEye]

The changes are going to completely mess up my libraries too, because some of the functions use the same sort of methods as halcmd eg

void HAL_Access::getAllPinNames(QStringList& list)
{
int next;
hal_pin_t *pin;
    rtapi_mutex_get(&(hal_data->mutex));
    next = hal_data->pin_list_ptr;
    while (next != 0) 
        {
        pin = (hal_pin_t *)SHMPTR(next);
        list.append(pin->name);
        next = pin->next_ptr;
        }
    rtapi_mutex_give(&(hal_data->mutex));
}

Once it is all settled will have to rewrite, so the insight will be useful.

I think halcmd completion just needs to go for now as you say. It can be ressurected once everything else is finished.

Have to admit I seldom use it, tend to do individual halcmd commands and recycle them through bash, rather than an interactive halcmd session.

[mhaberler]

yeah, I see the full impact now as well, quite something. Gtkscope, halmeter, Python/Cython/TCL bindings, halcmd . As I guessed.. bloodshed. The concept of abstraction never quite made it into HAL code;)

[ArcEye]

Thinking out loud here

From what you are saying, it occurs to me that what might really be needed is a higher level HAL set of functions that all other programs and sections of MK can use. (A proper C API)

If the 'library calls' were introduced first and they used the existing methods, all relevant code could be converted to use these calls and would work as before.

Then it is 'simply' a matter of changing what that call does. The user of the call does not need to know anything about the internals, just the parameters required and expected return.

halcmd has a lot of the functions you would need, but many of them printing to stdout. If those returned a definitive value or a list, that is what most other functions would need, without using the low level rtapi_ or even direct access.

Writing functions which returned lists was a some of what I did in my library for all the info functions, except QStringList's not zlists, they were very useful.

I wonder about the extent to which this could be applied in tandem to your current work.
Establishing one usage type common to many files, with each doing low level access and replacing with a function whose contents could later be converted to use the common headers.

...and indeed if you think that approach is worthwhile?

[mhaberler]

well my scope is more moderate, I would not go that far as introducing result sets/lists etc

also, the locking needs to be kept in mind - either you walk descriptors under the lock - consistent, or return a result set - only valid as long as lock held

for me it's removing the gazillion low-level accessors and iterators each of which assume full knowledge of the management method, which results in wads of hard-to-modify code

it looks like there will be a single iterator which doubles as a accessor, and only the special case of hal funct chains left out (this one is speed critical but small and isolated)

the other improvement will be locking granularity - the iterator/accessor halg_ methods can do it on a per-use basis, which makes writing bindings more robust

so right now I need to transmogrify halg_foreach() into cython ;)

have a look at the status:, it got some more access paths and keeps replacing the list walkers as I visit source files:

https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_object.h#L86-L138

eventually, removing all pins, functs and params of an instance wil become:

foreach_args_t args =  {
.owner_id  = <instance id>
};
halg_foreach(0, &args, free_object);

and exiting all pins, functs, and params owned by a comp (regardless of by instances or legacy) becomes:

foreach_args_t args =  {
.owning_comp  = <comp id>
};
halg_foreach(0, &args, free_object);

here are some selectors:

https://github.com/mhaberler/machinekit/blob/halobject-header/src/hal/lib/hal_object_selectors.h

As for C-level list API's.. I abstain. I leave that for Python - once halg_foreach is in Cython and robust, we can use the Python list/dict/object handling and Python is great at that

[ArcEye]

Didn't expect you to go for the C API:) It is effectively what I have and will have to re-write as things go along.

Most of the lists are not time crucial, they are used for graphical displays etc, but yes the actions based upon pin values etc. are done with the mutex held and will have to continue to be.

You have been busy, 27 changed files since I pulled this morning, including hal_completion.c, sio I will have a look at that

[mhaberler]

here's an idea for you, which might interest you since it's C++: think a bit about the remote HAL API.

Fact is - we dont have any formally, except my few articles and the protobuf defs.

But we have lots of operators and introspection through the protobuf/zeroMQ API. And: protobuf has arbitrarily nested submessages which are already used to describe HAL objects - pretty close to your lists idea, but more high-level: de facto an C++ object representation of HAL status (with the bonus of speedy to/from wireformat translation)

I found it a very good working API to deal with a remote representation.

What about a remote HAL inspector based on haltalk?

[mhaberler]

it may be a bit early to boast, but I think this has upside despite all the pain - I pushed a snapshot

tons of lowlevel code go out and the more goes out the less surprises remain. some legacy functions were reshaped (like the halpr_find_pin_by_sig iterator) and the major problems seem behind, we might even have a understandable HAL library ;)

[ArcEye]

 Cooking on gas!

Just had a quick look through and starting to see the patterns more
clearly, will look more closely at halcmd_commands.c later,
that should provide the best contrast for the differences for me.
 

[mhaberler]

I'm all for big burners, like the 5.2 Megawatts thingie here ;) (good British product btw, from Cameron Ballons, Bristol)

yes, the bigger impact is in the hal-near using code which so far assumed knowledge of internals

while I am working through it: dropping the hal/halpr distinction and making key methods halg_ will allow much easier handling of potential race condtions as composition of nested calls is not an issue any more

[mhaberler]

unfortunately I found another case of duplicate names: it's a group member's name and the signal it refers to, and that was of my own making

there are various ways how to fix this - probably the easiest one would be to get rid of the member object and make the signal directly a dependent object of the group; a refcount mechanism in the HAL common header is already in place to support that.

NB deleting the member object does not mean .hal files change - just the ops behind newm/delm . It would be invisible for compiled groups I guess.

What would be lost is a) epsilon b) the "userarg"

but since I'm working on generalized property lists for HAL objects these could stand in for epsilon and userarg, with the upside of not convoluting descriptors (same thing for component - userarg1 and userarg2)

so: for the time being - leave in until property lists merged, then replace by properties (delete member, comp.userarg*)

btw the refcounting scheme (in place for ALL halobjects) finally enables adding a pin to a group - the non-zero refcount would make comp exit fail until group deleted.

HAL shutdown logic needs adjusting for dependent refcounts on object deletion but that is automatic and behind the scenes

[mhaberler]

further to the previous issue - member vs signal name and duplicate names:

the easiest way out is to use "inheritance" .. a member is a signal plus some extra fluff. It contains the hal_signal_t struct as a struct element plus its extra fields - meaning signal semantics applies to members.

so example from machinetalk/tutorial/motorctrl:

now:

# HAL group example:
# monitor some signals via a group
newg dro 100
newm dro led
newm dro curr-pos
newm dro curr-vel
newm dro curr-acc

post-change:

# HAL group example:
# monitor some signals via a group
newg dro 100
newm dro led bit
newm dro curr-pos float
newm dro curr-vel float
newm dro curr-acc float

So creating a member implicitly creates a signal of the given type - a member is a subclass of signal