`QActive_psInit()`and `QActive_publish_()` currently "overlook" QPC reserved signals

[tiagonj]

The QActive_subscribe(), QActive_unsubscribe() and QActive_unsubscribeAll() functions correctly check that the signals in question are between Q_USER_SIG (inclusive) and QActive_maxPubSignal_ (exclusive) ✅ 

However:

• QActive_publish_() only checks that e->sig < (QSignal)QActive_maxPubSignal_ (sig < Q_USER_SIG shouldn't be allowed?), and
• QActive_psInit() (+ supporting global variables) is written in such a way that it forces subscrSto to be larger than it needs to be, by forcing it to provide storage for the QPC reserved signals.

To expand on this, it would actually be beneficial if QActive_psInit() allowed specifying both a minSignal and a maxSignal. This would not only be useful for carrying out the checks mentioned above (which currently implicitly assume minSignal == Q_USER_SIG) and allowing for a marginally smaller subscrSto (every small bit of memory helps on some targets), it would also allow client code to provide an extension to the reserved signals, something along these lines:

• QEP_EMPTY_SIG_ (0) <= sig < Q_USER_SIG QPC reserved signals
• Q_USER_SIG <= sig < minSignal extended reserved signals (entirely ignored by QPC framework)
• minSignal <= sig < maxSignal public signals, the only ones valid for qf_ps.c Publish-Subscribe services
• maxSignal <= sig private signals

If the client code doesn't have the need for any extended reserved signals then it would call QActive_psInit() with minSignal = Q_USER_SIG.

Such a change would also mean that the addressing of the QActive_subscrList_ array would not be done on e->sig anymore, but rather on e->sig - QActive_minPubSignal_.

[quantum-leaps]

Hi Tiagonj, Thank you for looking into the details of the publish/subscribe implementation and the suggestions to improve the API.

Yes, indeed, the 4 reserved signals are currently wasted in the subscription-table. But introducing the proposed minSignal incurs some runtime cost of calculating the index into the subscription-table. This recurring overhead is paid for every event publication, so it's not completely negligible.

Regarding the assertions in QActive_publish_(), the current check protects against indexing out of bounds into the subscription-table. The other checks you propose are redundant because they are performed in every event posting.

Finally, adding some reserved signals for the Application that QP should ignore is a complication of the API (even if you neglect that it breaks backward compatibility). Every additional parameter burdens the Application designer and creates opportunities for mistakes (if the parameter is wrong). Finally, the additional requirement for the QP framework to ignore additional signals creates overheads.

In summary, the proposed modifications don't pull their own weight, meaning that the dubious benefits don't justify the overheads and complications.

I really hope that my comments make sense to you.

--MMS

[tiagonj]

Hi Miro :wave: Thank you for the quick turnaround :slightly_smiling_face: :pray:

But introducing the proposed minSignal incurs some runtime cost of calculating the index into the subscription-table.

In QActive_psInit() one could offset QActive_subscrList_ to take this into account i.e.:

    QActive_subscrList_   = subscrSto - 4;          // (example, minSignal == 4)

such that the publishing of the public event with signal minSignal would resulting in addressing QActive_subscrList_[minSignal], corresponding to subscrSto[0].

And if the following remains true

The other checks you propose are redundant because they are performed in every event posting.

then we should never have to worry about a (negative) out-ouf-bounds indexing of subscrSto. This change, by itself, would be backwards-compatible because existing applications would still be providing the full N-sized array despite qf_ps.c only requiring an N-4-sized array.

Regarding the idea of allowing for extended reserved signals (which are meant to be entirely ignored by the QPC framework), I confess I'm not fully aware of all the ways in which this may break backwards compatibility, so I'm happy to take your word for it. I was hoping this wouldn't be the case because one would "only" have to make minSignal == Q_USER_SIG (NB: occurrences of Q_USER_SIG in qf_ps.c would be changed to refer to QActive_minPubSignal_ instead) to obtain the same behaviour as currently exists.

I completely agree this would amount to additional complexity (though would respectfully suggest the creates opportunities for mistakes is irrelevant in the context of well-tested software :slightly_smiling_face:) but I was throwing it out there in the hopes that it might be perceived as a natural and potentially desirable extension for some of the QPC projects out "in the real world". Full disclaimer: I work on one such project where this is being actively considered, because the alternative (adding such signals after the end of the public signal space) results in a much larger blast radius and is much less desirable from a technical point of view (think adding the QPC reserved signals after the public signals -- that type of headache). So the general thinking was: one can extend QActive or extend QEvt, why not extend also the QPC reserved signals.

Thank you again for the quick turnaround and your feedback! :pray:

-- Tiago

[quantum-leaps]

Hi Tiago, I must be completely missing your point, but I really don't get why you need the "ignored signals"? What's the purpose? What's the use case? Could you somehow justify such a feature request? --MMS

[tiagonj]

Hi Miro,

Apologies that I can't go into much detail (IP protection) but essentially we have a library of "QPC extensions" (think extensions to the QPC "classes", and related methods that go alongside them). For some of the intended functionality of those "QPC extensions" we require some signals that apply to the entire codebase (just like the QReservedSig signals), and we do not wish to create these as:

• public signals, because we don't want them to be publish-able (the same way that we don't want e.g. Q_INIT_SIG to be publish-able),
• private signals (the ones after the public signal space ends), because technical complexities make that a very undesirable choice (again: think about how nonsensical it would be to add the QPC reserved signals after the public signals -- that type of headache).

One approach is to inject these signals into enum QReservedSig (qep.h) (specifically, between Q_INIT_SIG and Q_USER_SIG but that carries a maintenance burden that we pay every time we upgrade to a more recent QPC version.

The next option is to leave the QPC source code unchanged (good :heavy_check_mark: ) and instead manually define those "QPC extensions" signals starting from Q_USER_SIG, in our own logic, at the expense of shifting the public signal space down by a few signals (perfectly doable :heavy_check_mark:). The only "catch" with this approach is that it increases the subscription-table memory waste (the first entries of QActive_subscrList_ which are never used, but the QPC framework forces the application logic to provide memory for) in qf_ps.c -- which is why I created this ticket.

-- Tiago

[quantum-leaps]

Hi Tiago, Thanks for the clarifications. In that case, my recommendation would be to allocate your special "application-reserved" signals at the top of the signal dynamic range. For example, if you use the default setting Q_SIGNAL_SIZE==2, you could define your own constant enum { MY_RESERVED_SIG = 0xFFFFU - 1000U };. Then you'll have 1000 "reserved" signals from the offset MY_RESERVED_SIG. Would that work for you? --MMS

[tiagonj]

Hi Miro :wave:

That definitely works :+1: Not sure if we'll choose this over the option I mentioned (starting from Q_USER_SIG, at the expense of more "wasted signals" in the subscription table) because of overall architecture reasons, but it's worth considering.

Thank you again for your input and insight! :pray:

-- Tiago