Bluetooth: HCI: Add utility to extract PB and BC flags from data header

[joerchan]

Add utility to extract the flags Packet Boundary and Broadcast to the
hci.h together with the rest of the ACL data header definitions.

Signed-off-by: Joakim Andersson [email protected]

[jhedberg]

Did you intend to use these somewhere in the code also? That seems to be missing. In the absence of actual use, you're failing to answer the most important question a commit message should answer, i.e. "why?" 😄

[jhedberg]

Could you fix the alignment to match the other lines? It seems you're using some tabs, which the other lines don't have

[joerchan]

The alignment seems to be correct. But sometimes the diff shows it differently, I think it is the + sign in front line that eats up some of the tab-width. Should I replace all the tabs and only use space perhaps? It's what I'd normally do but checkpatch requires 8 spaces to be tab.

[jhedberg]

All the existing bt_acl_* definitions use spaces for the alignment of the values, so please just follow that. The general rule we try to follow (since it makes presentation fairly consistent across different UIs) is tabs for indentation and spaces for alignment, i.e. tabs should only ever exist in the beginning of a line. And yes, as per Linux coding style (which Zephyr inherits) a tab width equals 8 spaces 😃

[joerchan]

Fixed. Thank you. Just assumed that it was tabs on the other ones.

I know the rule, but I have had issued with checkpatch complaining. But I think that was because I tried to align from the beginning of the line.

[joerchan]

Did you intend to use these somewhere in the code also? That seems to be missing. In the absence of actual use, you're failing to answer the most important question a commit message should answer, i.e. "why?" 😄

I'm using it here out of the zephyr tree.
https://github.com/NordicPlayground/fw-nrfconnect-nrf/pull/1532/files#diff-9c0368216e22de63fb64737226d07dd4R183

[jhedberg]

Sorry, one more thing (the alignment it fine now). To make the evaluation order completely clear, could you add an extra parenthesis around (h) >> 12 and (h) >> 14. E.g. I wouldn't know by heart if bit-shifts or binary operators take precedence over the other.

[jhedberg]

.. that'd actually also make the parenthesis usage consistent with the next line, i.e. the definition of bt_acl_handle_pack()

[Vudentz]

Btw this imply that the value would be extracted from the bare header and not after bt_acl_flags which is what we normally pass around.

[joerchan]

@Vudentz That is a good point. Actually looking here it seems like we should actually extract the separate flags. Because right now we use a combination and check with ==

	flags = bt_acl_flags(handle);
	handle = bt_acl_handle(handle);

	node_tx = ll_tx_mem_acquire();
	if (!node_tx) {
		BT_ERR("Tx Buffer Overflow");
		data_buf_overflow(evt);
		return -ENOBUFS;
	}

	pdu_data = (void *)node_tx->pdu;

	if (flags == BT_ACL_START_NO_FLUSH || flags == BT_ACL_START) {
		pdu_data->ll_id = PDU_DATA_LLID_DATA_START;
	} else {
		pdu_data->ll_id = PDU_DATA_LLID_DATA_CONTINUE;
	}

If it is best to have a 2 step get function I'm not sure. It seems convoluted to have to do pb_flag(flags(handle));

[Vudentz]

Maybe Im confusing with BlueZ then, I though the flags was passed over to another function as is, but yes it seems we are assuming the only pb is set when comparing so that would indeed need fixing, though that could be done with pb_flag(flags) if you don't want to create another variable to store them.

[joerchan]

@Vudentz You are right, we do pass around flags. So your suggestion was easier to use, mostly because we overwrite handle with flags masked out.

[jhedberg]

@joerchan I'm not completely sure it's appropriate for me to merge this during the stabilisation period since it's not a fix. On the other hand, it's trivial and I find it hard to imagine any way that this could cause regressions since the macros don't have any users upstream (for now). @dleach02 any thoughts?

[joerchan]

@joerchan I'm not completely sure it's appropriate for me to merge this during the stabilisation period since it's not a fix. On the other hand, it's trivial and I find it hard to imagine any way that this could cause regressions since the macros don't have any users upstream (for now). @dleach02 any thoughts?

No no, I wasn't expecting it either. Just creating the PR so that I don't forget this. It's a nice to have anyway so no rush.

[joerchan]

@jhedberg Now I'm using it :) The flags was not properly checked over HCI. Although the only issue I could see was that the BT controller would allow being sent a "flushable" packet. So it is not a big deal I think.

[carlescufi]

LGTM

[joerchan]

@jhedberg Can we merge this one?