hal: Rewrite most of halrmt to become functional and use the query API - #4352
hal: Rewrite most of halrmt to become functional and use the query API#4352BsAtHome wants to merge 1 commit into
Conversation
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
Verified build (clean, no warnings) and live-tested over TCP: HELLO/ENABLE gating, GET PINS/COMPS/THREADS/LOCK, async LOADRT with delayed ACK, SETP/SETS/NET/NEWSIG/DELSIG, ADDF/DELF, SAVE to connection and file, multi-client concurrency, QUIT, SHUTDOWN exit. All match old protocol semantics. Five small findings inline; none blocking. This is a strict improvement over the old non-functional halrmt.
| if(ctx.toks.size() > 3) { | ||
| // Filename was specified | ||
| int fd = creat(ctx.toks[3].c_str(), 0644); | ||
| if(!fd) { |
There was a problem hiding this comment.
creat() returns -1 on error, so if(!fd) never catches it (it only catches fd 0, which is a valid descriptor). Live test: SET SAVE net /bad-dir/file yields Write to 'net' failed ... errno=9 (Bad file descriptor) instead of the open error. Also, shouldn't this message name the filename (toks[3]) rather than the save key?
| if (tag->nset + 2 > ctx.toks.size()) { | ||
| errornl(ctx, fmt::format("Too few arguments to SET {}, have {}, need {} or more", | ||
| tag->name, ctx.toks.size() - 2, tag->nset)); | ||
| replynl(ctx, fmt::format("GET {} NAK", tag->name)); |
There was a problem hiding this comment.
This is the SET branch, but the reply says GET {} NAK. A strict client waiting on SET <cmd> NAK would desync here.
| // Still processing and waiting for the component to come online | ||
| // Check for component from program becoming ready | ||
| hal_query_t q = {}; | ||
| int rv = hal_comp_by_name(ctx.process.name_comp.c_str(), &q); |
There was a problem hiding this comment.
ready only checks that a component with this name exists and is ready. A duplicate SET LOADRT threads (or second loadrt siggen) gets SET LOADRT ACK even though rtapi_app refused the load ("threads: already exists"; halcmd fails this with an error). Should the child's exit status factor into the result when the comp was already ready before the load?
| Returns information about all components matching _pattern_. | ||
| If no _pattern_ is specified then all components are returned. | ||
|
|
||
| set delf <name>:: |
There was a problem hiding this comment.
SET DELF requires <funct> <thread> in the code (nset=2, hal_del_funct_from_thread(funct, thread)); the manpage shows only <name>, so following the doc NAKs.
| set timestamp {on|off} [{on|off}]:: | ||
| Enable or disable printing of a timestamp on messages sent back from the server to the user. | ||
| The optional format argument selects human readable date/time format and seconds since the epoch. | ||
| The format prints seconds when off and human readable when on. |
There was a problem hiding this comment.
This reads inverted relative to the code: timefmt ON prints epoch seconds and OFF prints the human readable form (halrmt.cc reply()).
2 participants
This PR is the rewrite of halrmt as required by the HAL updates. It is single-threaded now and supports multiple connections simultaneously. All direct access to HAL's inner workings have been removed and replaced with the HAL query API. The documentation has been updated.
There may still be some things that need be altered. The original code was mostly copied from halcmd and then forced into handling I/O via the network. Some operations do not necessarily benefit from that. As an example, the
SET SAVEcommand may include a filename, but that filename is on the remote computer (where halrmt runs). TheSET SAVEcommand can now run without the filename argument to output over the network connection, but the format is not suitable for putting it back into the network connection. The format does work with halcmd. It is necessary to rework the output so you can do full remote interaction without too many interpretation steps.There may still be other things lingering. However, this new version is an improvement and at least working, whereas the old version was deemed non-functional.
Note: The
halrmc.ccsource file is now a C++ file. Recompiling locally after merging may require amake cleanor at least the removal of the outdated dependency file(s) that still reference the oldhalrmc.cC-file. Otherwise a build error will note a missing file that should be missing but still gets referenced by a stale dependency.