Re: [PATCH 04/16 v2] Determine supported extended-remote features

Pedro Alves
2014-10-22T21:48:36Z

On 10/22/2014 12:23 AM, Breazeal, Don wrote:

> Hi Pedro,
> Thanks for looking at this. I should have made the rationale for
> the new qExtendedFeatures packet more clear in my original
> submission. I'll try to answer all of your questions in the
> explanation below.
>
> I had started out trying to report the features as
> supported/unsupported using qSupported, following the
> implementation of PACKET_multiprocess_feature as a model.
> I abandoned that approach when I saw these comments in
> remote.c_initialize_remote:
>
> /* Ideally all configs would have a command associated. Some
> still don't though. */
> [---snip---]
> case PACKET_multiprocess_feature:
> [---snip---]
> /* Additions to this list need to be well justified:
> pre-existing packets are OK; new packets are not. */
>
> I interpreted this to mean that the qSupported query was intended
> to be used only to enable/disable packets that mapped to commands,
> and that defining new packets that represented something else was
> discouraged.

Oh, you're reading this backwards --- this block is making sure that
we don't forget to call add_packet_config_cmd whenever we add a
new feature/packet. The commands that is talking about are the
"set remote foo-packet" commands that add_packet_config_cmd registers.
Those are commands that allow force- enable/disabling a given
RSP packet/feature. That loop is allowing some exceptions for
some packets/features that already didn't have the corresponding
"set remote foo-packet" commands when that assertion was
first added:

/* Assert that we've registered commands for all packet configs. */
{
int i;

for (i = 0; i < PACKET_MAX; i++)
{
/* Ideally all configs would have a command associated. Some
still don't though. */
int excepted;

See the log of ca4f7f8be, the commit that added this.

We can certainly add new qSupported features that don't map
to regular user commands.

Guess this would make it clearer:

- /* Assert that we've registered commands for all packet configs. */
+ /* Assert that we've registered "set remote foo-packet" commands for all packet configs. */

> In refreshing my memory about this I wrote up some detailed notes on
> it. Let me know if you want to see the gruesome details.
>
> Do you think the qSupported approach is preferable despite the
> comment? Did I misinterpret it?

Yes, qSupported is preferable. I'm sorry about not foreseeing
this potential confusion when I wrote that comment.

Thanks,
Pedro Alves