Expose indications and notifications in the att layer

[ivmarkov]

It just boils down to extending AttReq and AttRsp with some additional variants. NOTE: The attribute server is not (yet) updated to handle these - this should be a separate future PR, but this extension allows folks working with the att layer directly (or directly and then in a combination with the attribute server - quite possible!) to use these.

Possible topics of contention:

• I'm not sure Notify and Indicate should be part of AttRsp (while ConfirmIndication is also weirdly part of AttReq), as they are - strictly speaking - not a "response" to anything. The problem is this line of code which pretty much forces us to model these as "responses". On the other hand, we can still claim they are responses, yet - unsolicited (See GattData::reply_unsolicited.)
• We need a way to send indications and confirmations. Hence the new, "constructor"-less GattData::reply_unsolicited. By the way the Gatt server is not really using the AttRsp enum and is doing its own encoding; we should probably fix this in future
• I've also bravely put an #[allow(missing_docs)] // Temporary on top of the now-public att module. I'm also OK with commenting the module with a subsequent commit in this PR, but did not want the (small amount of) changes to be lost in the docu noise
• I really need cccd_handle to be public for my hybrid use of lower level att and higher level attribute server utilization and I made it so. Nothing wrong with that I hope?

[lulf]

It just boils down to extending AttReq and AttRsp with some additional variants. NOTE: The attribute server is not (yet) updated to handle these - this should be a separate future PR, but this extension allows folks working with the att layer directly (or directly and then in a combination with the attribute server - quite possible!) to use these.

Possible topics of contention:

• I'm not sure Notify and Indicate should be part of AttRsp (while ConfirmIndication is also weirdly part of AttReq), as they are - strictly speaking - not a "response" to anything. The problem is this line of code which pretty much forces us to model these as "responses". On the other hand, we can still claim they are responses, yet - unsolicited (See GattData::reply_unsolicited.)

Hmm, yeah. Maybe merging AttReq and AttRsp into Att is the way? Requires changing the variant names to something more in line with the name of the constant then perhaps.

• We need a way to send indications and confirmations. Hence the new, "constructor"-less GattData::reply_unsolicited. By the way the Gatt server is not really using the AttRsp enum and is doing its own encoding; we should probably fix this in future

The AttRsp was kind of a bolt-on for supporting gatt clients. I agree they should probably use the same. I think perhaps it's easier to fix now.

• I've also bravely put an #[allow(missing_docs)] // Temporary on top of the now-public att module. I'm also OK with commenting the module with a subsequent commit in this PR, but did not want the (small amount of) changes to be lost in the docu noise
• I really need cccd_handle to be public for my hybrid use of lower level att and higher level attribute server utilization and I made it so. Nothing wrong with that I hope?

I haven't really thought through the consequences of exposing it, but I guess it's ok with the caveat that maybe we will need to refactor/break this in the future. I'd be interested to know you have to use a 'hybrid' of the attribute server and the 'raw' request handling, is there something we can fix in the attribute server perhaps so that you wouldn't need the low level access?

[ivmarkov]

Hmm, yeah. Maybe merging AttReq and AttRsp into Att is the way? Requires changing the variant names to something more in line with the name of the constant then perhaps.

Sounds like a good idea! Let me quickly prototype this.

• I've also bravely put an #[allow(missing_docs)] // Temporary on top of the now-public att module. I'm also OK with commenting the module with a subsequent commit in this PR, but did not want the (small amount of) changes to be lost in the docu noise

Shall I also do this?

• I really need cccd_handle to be public for my hybrid use of lower level att and higher level attribute server utilization and I made it so. Nothing wrong with that I hope?

I haven't really thought through the consequences of exposing it, but I guess it's ok with the caveat that maybe we will need to refactor/break this in the future. I'd be interested to know you have to use a 'hybrid' of the attribute server and the 'raw' request handling, is there something we can fix in the attribute server perhaps so that you wouldn't need the low level access?

I think if we get to refactor the cccd_handle, it is very likely we'll refactor the already-public handle as well so I don't have big concerns there...?

[lulf]

Hmm, yeah. Maybe merging AttReq and AttRsp into Att is the way? Requires changing the variant names to something more in line with the name of the constant then perhaps.

Sounds like a good idea! Let me quickly prototype this.

• I've also bravely put an #[allow(missing_docs)] // Temporary on top of the now-public att module. I'm also OK with commenting the module with a subsequent commit in this PR, but did not want the (small amount of) changes to be lost in the docu noise

Shall I also do this?

I would really appreciate if it was documented before we made it public, which is the reason I've not made it public yet. I'm happy to do that, but you might have to wait a little with merging this until that is done as it's not a high priority task for me right now.

• I really need cccd_handle to be public for my hybrid use of lower level att and higher level attribute server utilization and I made it so. Nothing wrong with that I hope?

I haven't really thought through the consequences of exposing it, but I guess it's ok with the caveat that maybe we will need to refactor/break this in the future. I'd be interested to know you have to use a 'hybrid' of the attribute server and the 'raw' request handling, is there something we can fix in the attribute server perhaps so that you wouldn't need the low level access?

I think if we get to refactor the cccd_handle, it is very likely we'll refactor the already-public handle as well so I don't have big concerns there...?

No objections here, just as a warning if you're going to rely on it :)

[ivmarkov]

Hmm, yeah. Maybe merging AttReq and AttRsp into Att is the way? Requires changing the variant names to something more in line with the name of the constant then perhaps.

Sounds like a good idea! Let me quickly prototype this.

@lulf

Unifying AttReq and AttRsp into just Att is shaping out really nicely and I Iike it.

But. There is one single place I'm not sure how to handle, here:

trouble/host/src/host.rs

Line 404 in 0950a9c

match a {

Any suggestions?

If that's not clear, the question is a bit, shall we just trigger a GattEvent for all Att payloads, without differentiating whether that's a "request" or a "response"? Currently, for responses we don't do it but just send the payload somehow...

[ivmarkov]

Here's how it would look after the unification, except for that one pesky problem from above ^^^:
main...ivmarkov:trouble:single-att

[lulf]

Hmm, yeah. Maybe merging AttReq and AttRsp into Att is the way? Requires changing the variant names to something more in line with the name of the constant then perhaps.

Sounds like a good idea! Let me quickly prototype this.

@lulf

Unifying AttReq and AttRsp into just Att is shaping out really nicely and I Iike it.

But. There is one single place I'm not sure how to handle, here:

trouble/host/src/host.rs

Line 404 in 0950a9c

match a {

Any suggestions?

If that's not clear, the question is a bit, shall we just trigger a GattEvent for all Att payloads, without differentiating whether that's a "request" or a "response"? Currently, for responses we don't do it but just send the payload somehow...

I think that would break devices that operate as both gatt client and gatt server on the same connection, so I think for now it's probably best to add some method on the Att type to check if it's a request or response (I think you can just check for request code being an odd/even number) and dispatch to either channel based on that.

GattClient is probably due for a refactoring but it's better to do that later I think.

[ivmarkov]

@lulf Sorry for the delay.

I have updated the "single Att" branch (main...ivmarkov:trouble:single-att) with comments and with your suggested method that differentiates between client->server PDUs and server->client PDUs.

However, and after reading this (Section 3.3 to be precise), I think another option is to keep the current "Att Req/Rsp" dichotomy but maybe extend it a bit.

First of all, the Notification, Indication, ConfirmIndication and - as a matter of fact - the pre-existing WriteCmd PDU types do not fit cleanly into the Req/Rsp dichotomy simply because it is not really just two types of PDUs (Req and Rsp) - but - in fact 6:

• Commands, Requests, Responses, Notifications, Indications and (Indication) Confirmations;
• Further on, what you currently model with Att::Req (= opcode % 2 == 0) is actually all of Commands, Requests and (Indication) Confirmations, because opcode % 2 == 0 does not mean a request, but rather "a PDU generated by the client and to-be-processed by the server";
• Similarly, the current Att::Rsp (= opcode % 2 == 1) is all of Responses, Notifications and Indications, i.e. "a PDU generated by the server and to be processed by the client".

So this opens the door for two additional models (besides the "lets lump everything into a single Att enum" as I did in the above branch):

• Option 1: Simply rename Att::Req to - hm - Att::Client (or suchlike) and Att::Rsp to - say - Att::Server as this is - in fact - what they actually are
• Option 2: Do Option 1 but then additionally have the payload of Att::Client split into three additional enum variants:
  • AttReq <- most of the existing AttReq variants will go here
  • AttCmd <- This will be inhabited only by WriteCmd
  • AttCfm <- This will be inhabited only by IndCfm (the newly added Indication Confirmation)
  • Similarly, Att::Server / AttServer would be an enum with two variants:
    • AttRsp <- most of the existing AttRsp variants will go here except the newly-added Notification and Indication
    • AttUns <- This will be inhabited only by Notify and Indicate

"Option 2" would allow us to have a correct type for the new GattData::send_unsolicited which is necessary so that one can send unsolicited messages from the server to the client, which are exactly Notifications and Indications (i.e. send_unsolicited will take an AttUns param rather than the "lump-it-all-together Att). It would also allow us to have a better type signature for the existing GattData::reply which would take exactly AttRsp which would not include Notifications and Confirmations.

So, which one should it be?

• "lump it all in a single Att" -> pros - simpler; cons - we lose the strong typing of GattData::reply and the new GattData::send_unsolicited
• Option 1: pros - small changes (just a rename mostly) cons - we lose the strong typing of GattData::reply and the new GattData::send_unsolicited
• Option 2: pros - we preserve the strong typing; cons - a tad deeper enum-based hierarchy, but reflecting the ATT reality better perhaps?

[ivmarkov]

I have now updated this PR to actually implement "Option 2" as I felt this is the most correct path we should take.
Also, I have put the necessary documentation in-place so that we can expose the att module as a public one.

[lulf]

Looks good, just a few comments around the error handling. I agree with going for option 2. Thanks for documenting as well!

[lulf]

Somehow feels better to do a panic!() with a descriptive message here, or change signature to Result<AttClient<'_>, Error> and appropriate error.

[ivmarkov]

• I don't think we should change the API to be fallible, because at this place, it is impossible to get anything but Att::Client. This is enforced by this code in host.rs which posts into the connection event queue:

        #[cfg(feature = "gatt")]
        match a {
            /// !!! This branch specifically !!!
            Ok(att::Att::Client(_)) => {
                let event = ConnectionEventData::Gatt {
                    data: Pdu::new(packet, header.length as usize),
                };
                self.connections.post_handle_event(acl.handle(), event)?;
            }
            Ok(att::Att::Server(_)) => {
                if let Err(e) = self
                    .att_client
                    .try_send((acl.handle(), Pdu::new(packet, header.length as usize)))
                {
                    return Err(Error::OutOfMemory);
                }
            }
            Err(e) => {
                warn!("Error decoding attribute payload: {:?}", e);
            }
        }

Also, your comment // We know it has been checked, therefore this cannot fail in fact asserts exactly that (and the fact that the PDU has a valid format and can be decoded). It is just that in the past AttReq also had a public decode method, so the match+unreachable!() was not necessary. After my changes, only Att has a decode method (and I somehow believe this is cleaner), hence the additional unreachable.

• As for replacing panic! with unreachable!, an informal definition of unreachable! is given here. So I think it should be unreachable! rather than a panic!. Putting a message inside unreachable! is no prob though, I'll do that shortly.

[ivmarkov]

@lulf I've now documented the unreachable! statements more heavily, and I've put an error message inside unreachable!, if you can look again.

[lulf]

Same as earlier.

[ivmarkov]

See above - I'll put something in the unreachable!()

[ivmarkov]

@lulf Anything else to do here or would you say it is ready?
Note: I can still revert the unreachable! to panic! but if you could confirm you still want this, after the explanations I provided.

[lulf]

LGTM, thanks for explaining the unreachable thing, I think it's ok.

[lulf]

/test

[ivmarkov]

Is this a flaky test or did I manage to regress something?

[lulf]

/test

[lulf]

Is this a flaky test or did I manage to regress something?

Looks like some interference causing a disconnect. Retry seems to pass. Adjusting supervision timeouts etc. might improve stability, but this is the first time I've seen this one so can be fixed later.