Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share PDU code between CoAP and BLE? #63

Open
Jc2k opened this issue Feb 6, 2022 · 4 comments
Open

Share PDU code between CoAP and BLE? #63

Jc2k opened this issue Feb 6, 2022 · 4 comments

Comments

@Jc2k
Copy link
Owner

Jc2k commented Feb 6, 2022

I optimistically put the BLE PDU code in aiohomekit/pdu.py. But how generic is it?

We can move the CoAP versions for OpCode and PDUStatus to aiohomekit.pdu for sure.

In terms of merging encode_pdu:

  • How does CoAP handle IP fragmentation? The spec says to assume a max payload size of 1024. An ipv6 packet has a minimum MTU of 1280. So i'm guessing we won't need do do fragmentation for CoAP? So i don't really want to inflict my version on you if thats the case.

In terms of merging decode_pdu:

  • It looks like your CoAP accessory always returns a body length, even if there is no body. That's not the case for BLE. 3 byte response PDU's are pretty common. It will be interesting to see if that is a Nanoleaf behaviour or not.
  • The request/response bit should be set of BLE PDU, so I could use your version there.
  • I can see why you'd return a ValueError, but the return type gets a bit messy. I think for mine i'll probably add a "proper" exception rather than just a ValueError. Would that work for you?

CC @roysjosh

@Jc2k Jc2k added this to the 1.0.0 milestone Feb 6, 2022
@roysjosh
Copy link
Contributor

roysjosh commented Feb 6, 2022

I've never seen a fragmented payload. The largest response is the accessory database and it came in at 1108 bytes. We might have to wait for a device with multiple accessories or more services and characteristics to see what happens when it crosses ~1500 bytes. The Thread network fragments to ~100 bytes and the Border Router reassembles so maybe we would just get a big payload split across a few UDP packets...

I don't mind coding defensively on the decode, starting with 3 bytes & only decoding the length if present. Yeah, I'm not super happy with it, I was trying to find a way to decode multiple good PDUs that have an "invalid request" PDU in the middle & communicate that error condition back up among the good data. I thought I saw that behavior at some point though I can't find an example in my current dev log. Both the Home app and Nanoleaf app send multiple requests in a single payload which might be a difference to BLE?

@Jc2k
Copy link
Owner Author

Jc2k commented Feb 6, 2022

Yeah, you can't batch requests payloads with BLE. You have to know the right characteristic GATT endpoint to write to AND know the iid as well, so each PDU goes to a different GATT endpoint.

I guess if you have multiple PDU's in a single UDP packet the body length is actually mandatory? Otherwise how do you know if you've found a body or the next PDU payload. And it wouldn't make sense to have different behaviour for the last PDU in a payload.

@roysjosh
Copy link
Contributor

roysjosh commented Feb 6, 2022

I could just stick with bytes | PDUStatus and synthesize a PDUStatus for the tid mismatch and bad control conditions. That's slightly less gross.

@Jc2k
Copy link
Owner Author

Jc2k commented Feb 6, 2022

I was wondering the same thing, maybe make the value outside the range of a u8 so it doesn't clash in future?

@Jc2k Jc2k removed this from the 1.0.0 milestone Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants