andrzej-kaczmarek commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508759910 Note: to the best of my knowledge things work in NimBLE as I describe below, but there is very slight chance that some of numbers here are incorrect so please correct me with a concrete proof in such case. @haukepetersen We definitely should improve sanity checks to make sure configuration makes sense, especially if there are settings like `MSYS_1_BLOCK_SIZE` where we know that certain size is simply required so we do not try to read past allocated buffers. You can create an issue for this so we can continue discussion and perhaps someone can pick this up in spare time (perhaps some volunteer would like to gain a bit of knowledge on LL internals?). As for random failures and better error handling, I do not think there is any major issue in NimBLE in this regard. I mean, if we run out of buffers we don't just crash (if we do, this is an issue and not deliberate action) but try to do best we can that is allowed by spec, e.g. reply with an error or drop a packet (to be more precise: on RX we do not simply drop a packet, but we NAK it properly so it will be retransmitted). While it may seem like "random failures", this is not because of inherent randomness in NimBLE but due to Bluetooth traffic that is pretty much different every time you connect. Also, failures are usually logged to stats which can give a better understanding of what happens, but this is iirc not available in ports - feel free to port it :-) And now `MSYS_1_BLOCK_COUNT`: - 1 block is absolutely required for LL to start since it's allocated by scansm as a placeholder for SCAN_REQ PDU; by quickly looking through code I am pretty sure it would be possible to remove this requirement now since it should be possible to create SCAN_REQ on the fly (it was not possible when this was implemented few years ago) - 1 block is required to receive any LL packet and send reply if needed, because RX PDU block is reused for TX PDU in such case This means with only 2 blocks it is possible to create and maintain connection, and even receive some data. This is also enough for (G)ATT server to reply on requests in general, however reading attribute from ATT database requires an extra block for attribute value so some (most?) of those requests will fail with only 2 block available. With 3 blocks 80 bytes each you can have working connection with usable GATT (it works, I checked using `btshell`). So why 3 does not work for you? I do not know, could be that phone you try to connect with does some procedures which requires an extra block and it's just not possible to handle all control and data packets before e.g. timeout occurs. Each device behaves differently and that's basically why using an "absolute minimum" value is not really a good solution. Perhaps something like "recommended value" would be more appropriate here and this would be default value since they are adjusted in case we find current value is not good for some reason. One more example to give you better idea on why a single "absolute minimum" value won't work with all devices. While checking the above I found an issue in code handling `ATT Read By Group Type Request` where we can indeed silently drop packet inside ATT server due to oom. To be clear here, we do check this pretty consistently in code but there is single execution path where ATT error is not set properly which causes us to drop error response. This has an interesting consequence because it is actually what makes (G)ATT connection work with my Android phone using 3 blocks only. What happens after connection is that phone sends control PDU to update connection parameters and data packet with mentioned request - both consume remaining 2 blocks. We silently (and incorrectly) fail to reply on ATT request and apparently Android for whatever reason will retry after few seconds when LL procedure is completed so we have 2 blocks to handle request and it will work just fine. However, once I fix this issue (which is a simple one-liner) (G)ATT won't work because we will reply with an error on ATT request and phone will not retry so you basically can't discover services. So assuming I will fix the issue, what is an "absolute minimum" value here? I do not know, because I do not know how many other PDUs an Android phone can send us at once. As ambiguous as it sounds, "sane" is the proper value here. As for data length extensions, small blocks do not mean you can't use this feature. You can assuming there are enough small blocks to hold long data PDU - we do not need a single block to hold complete data PDU and we'll do just fine with chained mbufs. In fact, having a lot of smaller blocks instead of few large blocks may be better on nRF51 since it allows to use them more efficiently. Using large blocks, whenever possible, means we do not need to chain them which improves performance a bit but you usually waste RAM by using large block for small PDU. That said, since this applies to RIOT OS only I have no problem approving any values that are equal or above 3x80 (as this apparantly is what we can call "absolute mimimum" for legacy advertising + GATT over RAM transport scenario) - I am certain that what you select is tested and works for your purposes in RIOT OS so just let me know once you settled on final values in this PR. @simonratner PDU handling has changed. Previously scanner was using a raw data pointer to read advertising PDU payload thus it was required that complete PDU fits in single block (similar as for `CONNECT_IND` I described previously). Currently scanner has acccess to complete mbuf so it just uses raw pointer to access few header bytes (which is guaranteed if block size is large enough to fit `CONNECT_IND`) and payload is copied using mbuf API so it can be fragmented in mbufs chain.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
