[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-506650238 Hej everyone, is there anybody that could clarify the items above? I would like to go forward on this to finally enable `nrf51` support for NimBLE in RIOT... 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-507181065 @wes3 thanks for you feedback! Regarding my calculation: you are right about the access address, forgot about the nRFs easyDMA in-memory packet format... So taking this and the `struct ble_mbuf_hdr` into account, it should be: - 24 bytes mbuf overhead (sizeof(struct os_mbuf) + sizeof(struct os_mbuf_pkthdr)) - 12 bytes `sizeof(struct ble_mbuf_hdr)` - 39 bytes packet payload (w/o access address and w/o crc) Sums up to `75 bytes` minimal viable buffer size, right? > You could also restrict the default number of msys blocks; that will definitely help. Of course. But the question here as well, what would be the minimal viable value here? > Finally, if recollection serves, the 292 number comes from a maximum size PDU of 251 bytes using data length extension. I thought there was some documentation somewhere stating how that number was calculated. It certainly was not "random" :-) but without knowing exactly how it got calculated I could see how one would think that. The 'random' was of course meant very seriously :-) But I would be highly interested in the exact calculation that leads to this value, as between the max MTU of `251` and `292` there are still `41` bytes only partly accounted for. Since I work with NImBLE, the most trouble I had was related to buffer management, as NimBLE seems to be very sensitive about running out of them... So far, I simply generously over provision them, but this is of course only a temporary solution. My goal is to come up with some kind of elaborate documentation, that describes precisely how the buffers are used and how to adapt and configure them for ones specific use-case. I thought clarifying the above would be a good starting point :-) 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-507211954 Just gave this some tests using RIOTs `examples/nimble_gatt` example application, using the Nordic `nRF connect` Android app as GATT client. I deployed it on two different targets, `nrf52dk` and `microbit (nrf51)`. The bare minimum numbers, for which the GATT server was still working were - `MYNEWT_VAL_MSYS_1_BLOCK_SIZE := 77` - `MYNEWT_VAL_MSYS_1_BLOCK_COUNT := 4` For any block-size smaller than `77 bytes` I can't connect to the GATT server, so there must be something still missing in the numbers above?! For any block count <4 the phone can not connect to the device. I tried to figure out why 4 buffers are needed, but I am unsure. So in the most busy case, I get: - 1 mbuf allocated by the controller to receive the next packet - 1 mbuf allocated by the controller for keeping an empty packet to reply on the next connection event in case there is no GATT payload to be send - 1 mbuf holding data currently processed by the GATT server - 1 mbuf allocated by the GATT server for sending a reply? 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508433856 Slowly I start to see some light in the end of that tunnel :-) The 80 bytes minimum size for `MSYS_1_BLOCK_SIZE` makes sense and is now clear to me. I will adapt the value accordingly. However, I think the error handling in the NimBLE code could be vastly improved - having to actually now about these specifics from a user perspective is quite a lot to ask. And as for my experiences: when badly configuring buffers you end up with a silent fail, which is quite frustrating... > however there are few places in code which assume that complete PDU is available in first mbuf that is exactly one of those assumptions that is IMHO not handled very user friendly in case of bad configuration :-) > As for minimum number of blocks, I do not know what is the absolute minimum required [...] That does not sound good. For sure there can't the one general number that is valid for all the different possible transports etc, but for the specific use-case of running GATT over RAM transport using the NimBLE controller, I would think that there is value that can be stated. Not talking about degraded performance here (which is a tradeof a user has to chose), but a safe value that will at least not let the stack crash. > [...] but anyway I do not recommend to set this to such value since it can simply make things fail randomly. This comes pretty much to the core of the problems I was having: NimBLE has a tendency for random failures when it runs out of buffers. To me, this is unacceptable! I would rather expect NimBLE to (more or less silently) drop packets, or at least fail using assertions when in devel mode. For now I can live with simply setting this value to something I found reasonable in testing, but it does not feel right with without being able to justify exactly why this value is working and the value-1 is not... In general, I would very much like to see some improvements regarding the memory management: - better documentation about how to configure the buffers for selected usecases (transports, protocols used, ...) - better error handling inside the host/controller when running out of buffers I will gladly help to do this as far as I can, but there are apparently a lot things that exceed my knowledge :-) 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508668178 This discussion is quite fruitful for me, and I hope I don't start to annoy anyone :-) To go forward, I'd like to suggest the following: - get this PR merged with the current values (80 bytes msys bufsize, 4 msys buffers) -> so is it ok if I squash? - move/continue this discussion in a separate issue -> I will gladly open one an summarize what we discussed so far. Only I am offline on holiday for the next 3 weeks, so you won't hear back from me until August... 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508669650 @simonratner my browser did not refresh in properly, so I only saw your comment just now. Could you maybe elaborate, where those 'new' 4 byte (for non extended advertising) come from? Adding the additional 4 byte to cater for extended advertising as safety margin does indeed not hurt -> adapted the PR accordingly. 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508676739 Oh, I am too tired this morning, everything I needed is actually in the email archive posted above... To summarize (one again), for the current master, the numbers seem to be (just tested on a `nrf51` cortex-m0): - `sizeof(struct ble_mbuf_hdr)` -> 16 bytes / 20 bytes (w and w/o extended adv) - `sizeof(struct os_mbuf)` -> 16 bytes - `sizeof(struct os_mbuf_pkthdr)` -> 8 bytes - HCI ACL data header: 4 bytes - PDU: 39 bytes -> sums up to 83 bytes / 87 bytes -> 88 bytes still seem valid with the current code base. 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-508702215 so from what I learned, I further disabled the controller's data length extension by default for the RIOT port. As the `msys` buffer sizes are in the default config now fairly minimal, it seems to contradicting to allow for large link layer packtes at the same time. Also it fits better into the overall integration strategy for RIOT: per default, NimBLE provides a minimal configuration, and the RIOT build system takes care of (re)enabling all the features on demand. 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-517207749 sorry for the late reply - the long holiday was very nice :-) First of all, thanks again for the detailed explanations, this is very helpful. Also thanks for the improvements in #509. I can verify, that the `3x88` configuration is now working with my phone and both nrf51 and nrf52 based boards. I can however also observe the ATT issue pointed out above. Once again, the Bluetooth standard is allowing behavior that is not very nice for (very) contrained devices... To settle this PR, I decided I'd like to go with 5 blocks of 88byes each as default configuration (according changes are force-pushed to this PR). This seems to be a good compromise allowing a little bit of variance concerning GATT/ATT behavior while still keeping memory consumption low enough for nrf51 targets. 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
[GitHub] [mynewt-nimble] haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum
haukepetersen commented on issue #477: npl/riot: reduce default MSYS_1_BLOCK_SIZE to minimum URL: https://github.com/apache/mynewt-nimble/pull/477#issuecomment-517244328 awesome, thanks for sticking with me :-) 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
