Hi Marcel, On Wed, May 28, 2014 at 11:37 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote: > Hi Arman, > >>>> +unsigned int bt_att_send_sequential(struct bt_att *att, uint8_t opcode, >>>> + const void *param, uint16_t length, >>>> + bt_att_request_func_t callback, void *user_data, >>>> + bt_att_destroy_func_t destroy); >>> >>> That is the implied behavior for _send functions. So I would not use _send_sequential here at all. >> >> This mainly came about after the discussion with Luiz on patch set v1. >> My idea was to use bt_att_send for non-sequential ops and >> bt_att_send_sequential for the opposite. I could rename these to >> bt_att_send_without_rsp and bt_att_send, respectively. > > I am getting the feeling that just having bt_att_send only seems much cleaner. And if you give an opcode that does not expect a response, then you have to give an empty callback function. If not you get an error. I have to scratch my head a bit against this. > >>> I am still not sure that we should be doing it like this. We tried this with gattrib and it did not turn out that great. I really like the approach of I send this command and expect this PDU back or an error. The reason here is that in theory you can get interleaved commands/response on the same channel. >> >> I understand the reasoning behind this; then again it introduces >> potential error cases such as expecting the wrong PDU back, e.g. >> expecting a "Write Response" while sending a "Read Request". In the >> end, I don't see much value in building the API that way, since in >> ATT, all sequential requests have a 1-to-1 mapping to a sequential >> response. e.g., if we receive a "Read Response", the currently pending >> request absolutely has to be a "Read Request", otherwise we are >> supposed to ignore the PDU (or drop the connection entirely, according >> to the spec). >> >>> >>> So this is what you would normally expect: >>> >>> -> command >>> <- response >>> >>> Straight forward and simple, right. However nothing is stopping the other side from issues the same command in the other direction. >>> >>> -> command >>> <- command >>> <- response >>> -> response >>> >>> And this is not to mention notifications and confirmations that can happen as well. >>> >>> I am thinking out loud here. What might help is a table that classifies which opcode is a command, response, notification or confirmation and based on that we just do the right thing. >> >> Well, I actually did write a similar unit test on my local branch >> using my current implementation and these work as expected. The way I >> structured the code actually does (or will do in upcoming patches) >> exactly what you say, though instead of using tables, each "handler" >> function explicitly checks the opcode to see if it falls into its >> expected category (e.g. handle_response, handle_notification, etc.). >> >> ATT is a really simple protocol in this regard. > > It is simple, but it is also not structured. Opcode numbers have no higher meaning. They are just numbers. That why having a table of known opcodes and its semantics might be the simplest way. > >>> >>> Unfortunately we need to get this one right and I know that rebasing patches can be a pain. So if you just want to send the initial 01/01 patch for this then that is fine with me as well. I think some units tests with this overlapping bi-directional communication would be amazingly great to have. I do wonder if other OSes handle this well. >> >> Absolutely. Let me know if this has convinced you. So my idea is to >> have a bt_att_send and bt_att_send_without_rsp. I originally had a >> single bt_att_send for all opcodes, but Luiz brought up the fact that >> having a one for all function leads to some inconsistencies, such as >> cases where a callback is passed along with an opcode that doesn't >> expect a response. Personally I don't think this is such a problem as >> long as the API is well documented. > > Right now as said above, just having bt_att_send seems enough. Start with that and see how it goes. Add an extra simpler helper that utilizes bt_att_send with an empty callback is dead simple. We can worry about that later. > >> Luiz's suggestion was to actually have a separate function per ATT >> opcode. In that approach we would remove the opcode + parameter >> structure design entirely and have the individual functions accept the >> right parameters. So, no bt_att_send, but bt_att_send_read_request, >> etc. Do you think this would be a cleaner approach in the end? > > I do not want this. Having separate function for each opcode pair or function gets really complicated. We have been down this rabbit hole before. The more generic approach have worked out nicely with mgmt handling and I really like trying to duplicate that. And my current feeling is that bt_att_send is the way to go. At least the responses seems useless to queue to write latter and then auto acknowledge once written because that will just cause more latency, memory and complexity and we can probably just return synchronously and retry/fail if a response cannot be sent out, we have to remember that there is a socket in between so it is very unlikely that a write would fail and in case that fail it is actually better to return and error than just keep queuing. In case of MGMT perhaps it was different because there you don't actually have responses but actual requests that needs to be acknowledged before the next request can be sent out. -- Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
