hal: add pushmsg component for RT message generation#4003
hal: add pushmsg component for RT message generation#4003grandixximo wants to merge 1 commit intoLinuxCNC:masterfrom
Conversation
|
How does this compare with the existing "message" component? https://linuxcnc.org/docs/stable/html/man/man9/message.9.html |
The message component only allows you to send error messages. You cannot use it for debug, warning or informational messages. |
|
@ BsAtHome While we are at it, what about, .msg or similar file, INI style and lift the 16 max limit? Or is this too complex? |
grandixximo
force-pushed
the
feature/pushmsg-comp
branch
from
6553f80 to
a87a084
Compare
|
Reading files in components is not necessary here. It should suffice to enter them as arguments in the loadrt. The problem I have is the loop as such (I wrote exactly the same thing before deciding it was wrong). I think it is wasteful to loop over all possible triggers that are all ever so unlikely until you actually trigger. Similarly, having a single trigger and a setup pin to select the message makes it more difficult to select which message you want to emit. Neither solution are enough KISS in my eyes. At least, I'm not convinced either solution is the right one. |
|
I don't think no loop is structurally possible if input is N HAL bit signals... |
I find that keeping track of which message is which on a single long string of loadrt is quite a bother, that is why I was proposing a file, where everything is forcibly formatted to be readable and properly indexed, knowing what message 7 of 15 is at a glance is convenient when you are debugging, rather than counting the commas in a long string. |
|
That is a valid argument. However, you then need to parse a text file in the kernel... |
|
Fair, no kernel parsing needed, if what we want is just a better messaging tool for integrators. I was thinking of rescoping to a userspace daemon (loadusr): reads config file, creates HAL pins, polls at 20 Hz, emits via rtapi_print_msg (all 4 levels) plus NML for GUI ERR. RT untouched, file remains readable, <halpin.x> substitution available. What do you think? |
Well, that would make this whole discussion redundant. The point of the original hack was to test rtapi_print_msg() propagation from within a component in RT space. Making a userspace daemon is kinda like a "problem solved, patient dead" solution. I am looking for the actual use-case where you want several messages at several levels. The "old" component |
|
Fair, uspace message is another PR, not rescoping this one, |
|
Well, answer my question: What is the use-case? There must be a clear reason why this is more than just a debug tool. And then, why use a loop over all message types? Wouldn't it be better to use two args, one with strings and the other with message level? This complicates pins, but reduces cpu cycles. I have this cost/effort/benefit picture in my mind that does not want to fit. |
|
Use-case: integrator commissioning + field diagnostics. Real examples I hit:
The
A 4-level RT push closes that gap. NML is GUI-only; rtapi_print_msg reaches dmesg/log too, which matters for headless/embedded retrofits. On design: agreed, drop the per-level loop. Two parallel modparams, Bonus: HAL pin value substitution via Example: Want me to redraft |
|
Maybe it would be better to have a string of key-value pairs in the loadrt line? |
|
I think that the "message" component is used quite often on real-world configs. Whether is should be used in a sample config is an interesting question. |
|
And we can make it easier for people by using an INI expansion like: And then: [PUSHMSG]
# Note: no surrounding whitespace
MESSAGES=\
"w|oil|Low oil",\
"e|nooil|No oil",\
"i|ping|Tool said hi" |
grandixximo
force-pushed
the
feature/pushmsg-comp
branch
from
a87a084 to
294fb90
Compare
| typedef struct { | ||
| hal_bit_t **trigger; | ||
| hal_bit_t prev; | ||
| int level; | ||
| char literal[MAX_LIT]; | ||
| int litlen; | ||
| ref_t refs[MAX_REFS]; | ||
| int nrefs; | ||
| } slot_t; |
There was a problem hiding this comment.
There are two types of structures required. One where the hal reference lives (in hal memory) and one for internal admin can be simply allocated.
The hall pin in not a double pointer. It is a single pointer.
You do not use hal-types to store the previous pin status. This is abuse of hal types and we've been battling this for a long time. You need the underlying type.
With these very basic mistakes, it looks like this code is LLM generated??
There was a problem hiding this comment.
Reworked, apologies for the rough first pass, not sure I fully understand why it is wrong :-(
I did have LLM help, but I don't understand why its wrong, I'll look at it again tomorrow after some sleep...
There was a problem hiding this comment.
Spent the morning studying it, I think I get it now....
HAL pin storage and admin state in separate structs (paired by index). Mixing forces per-pin malloc instead of one alloc sized after pre-count, which should be preferred.
Internal state uses underlying C types (bool, int), not hal_bit_t.
I had the wrong mental model on the double pointer; thought it was a clever address cache. The legitimate ** pattern (matrix_kb, classicladder, mux_generic) is pointer-to-array with one alloc and indexed access. Per-pin ** is just single-pointer with extra steps.
Noticed several legacy components mix the two struct types (encoder, pid, pwmgen, stepgen, parport, debounce, watchdog) and mb2hal does the per-pin malloc, so I see the pattern you've been pushing back on. Not asking to clean those up, just confirming I get it?
There was a problem hiding this comment.
Hal pins in a component are a pointer to a in-HAL-memory data unit. When you create a pin, then you must store the reference to that internal data unit. The reference is a pointer. To store a reference, you need the address of the reference, hence the double pointer.
Yes, there is a lot of confusion about types and memory usage. It needs to be cleaned up, but that is another issue.
grandixximo
force-pushed
the
feature/pushmsg-comp
branch
3 times, most recently
from
2036f36 to
ff3e7fb
Compare
pushmsg accepts a list of `level|pinname|message` entries via the `msgs` modparam and creates one `pushmsg.<pinname>` HAL trigger pin per entry. On the rising edge the corresponding text is emitted via rtapi_print_msg() at the configured level (e/w/i/d). Message text may include `<refname:%fmt>` placeholders. Each placeholder exposes a typed input pin `pushmsg.<pinname>.<refname>` that the user nets to the source signal. Type is inferred from the format spec (`%d`/`%i` => s32, `%u`/`%x` => u32, `%f`/`%g`/`%e` => float, `%b` => bit). The cycle function only dereferences the component's own pins; no HAL search, no locking, no kernel string parsing in RT. HAL state and admin state live in separate structures: pin pointer storage is one hal_malloc block sized to the parsed entry count, edge state and parsed literals stay in module memory. A master `pushmsg.enable` pin gates emission for runtime suppression. Maximum 64 entries per loadrt, 8 references per message.
grandixximo
force-pushed
the
feature/pushmsg-comp
branch
from
ff3e7fb to
dcf0c95
Compare
|
I've been hacking a bit too (with my LBM† running on sugar and oxygen) and came up with this: † Large Brain Model ;-) component pushmsg "Push a message to non-RT";
pin in bit enable "Enable message generation";
description """
bla bla bla
"""
;
modparam dummy msgs "Comma separated list of custom messages in format t|pin|message";
option period no;
option extra_setup yes;
option extra_cleanup yes;
option singleton yes;
function _;
license "GPL"; // v2+
author "LinuxCNC";
include <rtapi_ctype.h>;
include <rtapi_gfp.h>;
include <rtapi_slab.h>;
;;
#define MSG_LEN_MAX 128 // Max size of a message at input and expanded
#define MSG_N_MAX 64 // Max number of messages supported
#define MSG_SUBST_MAX 2 // Max number of substitutions within one message
// Command-line config parameter
const char *msgs[MSG_N_MAX] = {};
RTAPI_MP_ARRAY_STRING(msgs, MSG_N_MAX, "Message slots in 't|pin|message' format");
// The HAL structure lives in HAl memory space
typedef struct {
hal_bit_t *trigger;
hal_data_u *substs[MSG_SUBST_MAX];
} msg_hal_t;
// The admin structure lives in normal memory space
typedef struct {
bool prev; // Previous pin state (to detect rising edge)
int msgtype; // Rtapi message type (err, warn, info, debug)
char msgbuf[MSG_LEN_MAX]; // Complete message with embedded NULs for substitution
int msglen; // Length so we may find the end easily (stpcpy is not available)
hal_type_t substtype[MSG_SUBST_MAX]; // Substitution types (hal type enum)
const char *parts[MSG_SUBST_MAX]; // Text parts index msgbuf for after substitution
} msg_slot_t;
//
// The message structure is decomposed from the source:
// msgs[n] = "e|trigpin|Message {s:pin1} othertext {u:pin2} trailing"
//
// The source is copied into the slots->msgbuf and all pin refs are isolated:
// msgbuf = "Message \0s:pin1\0 othertext \0u:pin2\0 trailing"
// ^ ^
// parts[0] parts[1]
// The slots->parts[] array is always the trailing part after the substitution.
// The pins for the substitution variable are referenced in the pins->substs[]
// array.
//
static msg_hal_t *pins; // Trigger and substitution pins
static msg_slot_t *slots; // Admin data for each message
static int nslots; // Number of active messages
static inline char printable(char c)
{
return isprint(c & 0xff) ? c : '?';
}
static int message_type(char c)
{
switch(c) {
case 'e': case 'E': return RTAPI_MSG_ERR;
case 'w': case 'W': return RTAPI_MSG_WARN;
case 'i': case 'I': return RTAPI_MSG_INFO;
case 'd': case 'D': return RTAPI_MSG_DBG;
}
return -1;
}
static int check_pinname(const char *name)
{
for(;*name; name++) {
if(!isascii(*name & 0xff) || (!isalnum(*name & 0xff) && NULL == strchr("._-", *name & 0xff)))
return -EINVAL;
}
return 0;
}
static int setup_message(const char *pfx, int idx, const char *msg, msg_slot_t *slot, msg_hal_t *pin)
{
int pfxlen = strlen(pfx);
int l = strlen(msg);
if(l >= MSG_LEN_MAX) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d too long, more than %d characters\n", pfx, idx, MSG_LEN_MAX-1);
return -EMSGSIZE;
}
// Absolute minimum is "t|p|"
if(l < 4) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d too short, less than 4 characters\n", pfx, idx);
return -EMSGSIZE;
}
// Get the message type
if((slot->msgtype = message_type(msg[0])) < 0) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d has invalid type '%c', expected one of {e,w,i,d}\n",
pfx, idx, printable(msg[0]));
return -EBADMSG;
}
// Message type must be followed by a '|'
if('|' != msg[1]) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d missing first '|' separator\n", pfx, idx);
return -EBADMSG;
}
// Find the second '|' in "t|name|message"
const char *pinend = strchr(msg+2, '|');
if(!pinend) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d missing pin name, no second '|' separator\n", pfx, idx);
return -EBADMSG;
}
// Make sure we can encompass the full name
int pinnamelen = pinend - msg - 2;
if(pinnamelen + pfxlen + 1 >= HAL_NAME_LEN) { // +1 for the extra '.' character
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d pin name too long\n", pfx, idx);
return -EBADMSG;
}
int rv;
// Copy the pinname for later pin creation use
char pinname[HAL_NAME_LEN+1] = {}; // The init ensures termination
memcpy(pinname, &msg[2], pinnamelen);
if((rv = check_pinname(pinname)) < 0) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d pin name contains bad characters\n", pfx, idx);
return rv;
}
// Create the trigger input pin
if((rv = hal_pin_bit_newf(HAL_IN, &pin->trigger, comp_id, "%s.%s", pfx, pinname)) < 0) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d pin name too long\n", pfx, idx);
return rv;
}
strcpy(slot->msgbuf, msg + pinnamelen + 3); // Copy message so we may take it apart
// Now see if there are any expansions in the string.
int subst = 0; // Count number of substitutions
for(char *cptr = slot->msgbuf; *cptr; cptr++) {
// Check "\{..." escaped curly open brace
if('\\' == *cptr) {
// The next char is escaped and cannot start a substitution
if(cptr[1]) // Or this is a backslash at the end
cptr++;
continue;
}
if('{' == *cptr) {
// Need "{t:pinname}"
*cptr = 0; // Terminate the previous string "foo {t:pin} bar" --> "foo "
cptr++; // Move to subst internals
char *end = strchr(cptr, '}');
if(!end) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d missing '}' in substitution\n", pfx, idx);
return -EBADMSG;
}
slot->parts[subst] = end + 1; // This is the continuation position "t:pin} bar" --> " bar"
*end = 0; // Terminate the pinname "t:pin} bar" --> "t:pin"
if(end - cptr - 1 < 3) { // Note: end points one char beyond (at a NUL)
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d substitution too small, need type and pinname\n", pfx, idx);
return -EBADMSG;
}
if(':' != cptr[1]) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d substitution missing ':' after type character\n", pfx, idx);
return -EBADMSG;
}
cptr += 2; // Move to pinname
// Note: end points to the trailing char
if((end - cptr - 1) + pfxlen + 1 >= HAL_NAME_LEN) { // +1 for the extra '.' character
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d substitution pin name too long\n", pfx, idx);
return -EBADMSG;
}
if((rv = check_pinname(cptr+2)) < 0) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d substitution pin name contains bad characters\n", pfx, idx);
return rv;
}
// Create the correct pin based on the type letter
// Unfortunately, we cannot determine whether a pin has a duplicate
// name when pin creation fails. The return value is not specific
// enough. We'd have to track all pin names ourselves if we wanted
// to merge pin substitution references into one pool.
switch(*cptr) {
case 'b': case 'B':
slot->substtype[subst] = HAL_BIT;
if((rv = hal_pin_bit_newf(HAL_IN, (hal_bit_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
case 's': case 'S':
slot->substtype[subst] = HAL_S32;
if((rv = hal_pin_s32_newf(HAL_IN, (hal_s32_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
case 'u': case 'U':
slot->substtype[subst] = HAL_U32;
if((rv = hal_pin_u32_newf(HAL_IN, (hal_u32_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
case 'l': case 'L':
slot->substtype[subst] = HAL_S64;
if((rv = hal_pin_s64_newf(HAL_IN, (hal_s64_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
case 'k': case 'K':
slot->substtype[subst] = HAL_U64;
if((rv = hal_pin_u64_newf(HAL_IN, (hal_u64_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
case 'f': case 'F':
slot->substtype[subst] = HAL_FLOAT;
if((rv = hal_pin_float_newf(HAL_IN, (hal_float_t **)&pin->substs[subst], comp_id, "%s.%s", pfx, cptr)) < 0)
return rv;
break;
default:
rtapi_print_msg(RTAPI_MSG_ERR, "%s: Message %d bad substitution type, expected one of {b,f,s,u,k,l}\n", pfx, idx);
return -EBADMSG;
}
// Do not perform more substitutions than allowed
subst++;
if(subst >= MSG_SUBST_MAX)
break;
cptr = end; // Move to end of substitution (loop will increment to next char)
}
}
// Finally set the first part's length for quick concat in print_slot()
slot->msglen = strlen(slot->msgbuf);
return 0;
}
EXTRA_SETUP()
{
(void)__comp_inst;
(void)extra_arg;
// See how many messages are set
for(nslots = 0; nslots < MSG_N_MAX && msgs[nslots]; nslots++) {
// Just counting. Drops out at the first NULL (unset msgs)
rtapi_print_msg(RTAPI_MSG_DBG, "%s: msgs[%d]=%s\n", prefix, nslots, msgs[nslots]);
}
if(nslots < 1) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: No messages to handle\n", prefix);
return -EINVAL;
}
// Get memory for the HAL interface
pins = (msg_hal_t *)hal_malloc(nslots * sizeof(pins[0]));
if(!pins) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: No HAL memory for pins\n", prefix);
return -ENOMEM;
}
memset(pins, 0, nslots * sizeof(pins[0])); // Ensure zeros because we check NULLs
// Get memory for the message slots
slots = (msg_slot_t *)rtapi_kzalloc(nslots * sizeof(slots[0]), RTAPI_GFP_KERNEL);
if(!slots) {
rtapi_print_msg(RTAPI_MSG_ERR, "%s: No kernel memory for slots\n", prefix);
return -ENOMEM;
}
// Interpret every message format and allocate pin resources
for(int i = 0; i < nslots; i++) {
int rv = setup_message(prefix, i, msgs[i], &slots[i], &pins[i]);
if(rv < 0) {
rtapi_kfree(slots);
slots = NULL;
return rv;
}
}
return 0;
}
EXTRA_CLEANUP()
{
if(slots)
rtapi_kfree(slots);
}
static void print_slot(int s)
{
char buf[MSG_LEN_MAX];
char *end = strcpy(buf, slots[s].msgbuf) + slots[s].msglen;
for(int i = 0; i < MSG_SUBST_MAX && pins[s].substs[i]; i++) {
int left = sizeof(buf) - (buf - end);
int n;
if(left <= 1) // Need room for terminator
break;
switch(slots[s].substtype[i]) {
case HAL_BIT:
n = rtapi_snprintf(end, left, "%d%s", (int)!!pins[s].substs[i]->b, slots[s].parts[i]);
break;
case HAL_S32:
n = rtapi_snprintf(end, left, "%d%s", (int)pins[s].substs[i]->s, slots[s].parts[i]);
break;
case HAL_U32:
n = rtapi_snprintf(end, left, "%u%s", (unsigned)pins[s].substs[i]->u, slots[s].parts[i]);
break;
case HAL_S64:
n = rtapi_snprintf(end, left, "%ld%s", (long)pins[s].substs[i]->ls, slots[s].parts[i]);
break;
case HAL_U64:
n = rtapi_snprintf(end, left, "%lu%s", (unsigned long)pins[s].substs[i]->lu, slots[s].parts[i]);
break;
case HAL_FLOAT:
n = rtapi_snprintf(end, left, "%lf%s", (double)pins[s].substs[i]->f, slots[s].parts[i]);
break;
default:
n = 0;
break;
}
if(n < 0) {
rtapi_print_msg(RTAPI_MSG_ERR, "pushmsg: Printing slot %d failed rtapi_snprintf() call\n", s);
return;
}
end += n;
}
rtapi_print_msg(slots[s].msgtype, "%s\n", buf);
}
// Servo-thread cycle function
FUNCTION(_)
{
bool en = enable; // Cache enable pin
// For each slot, test the trigger and save the trigger state
for(int i = 0; i < nslots; i++) {
bool trig = *(pins[i].trigger);
if(en && trig && !slots[i].prev) {
print_slot(i); // Rising edge -> print message
}
slots[i].prev = trig;
}
}
// vim: ts=4 sw=4 syn=c |
3 participants
KISS RT-to-non-RT message generator, replaces the dirty test-rig hack used in the #3993 latency comparison.
Each rising edge on
pushmsg.{error,warning,info,debug}-Nemits the corresponding string from the load-time{e,w,i,d}msgarrays throughrtapi_print_msg(). 16 slots per level. Slots without a configured message are inert. Singleton instance.Example:
cc @BsAtHome who proposed this in #3993.
Draft for discussion. Open to flexibility tweaks (personality bits for per-level counts, multi-instance, etc.).