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

Fix cut off scan buffer. #740

Merged
merged 1 commit into from Feb 8, 2018

Conversation

Projects
None yet
5 participants
@torfmaster
Contributor

torfmaster commented Feb 5, 2018

Pull Request Overview

This pull request fixes a bug in the ble-passive-scanning functionality for the nrf52 board.
The driver cut off the last byte of the advertising packet in the buffer, so e.g. service data was lost.

Testing Strategy

It was tested manually in userspace by sending packets with service data/manufacturer data with different lengths. Service data is now copied to the scanning buffer completely.

TODO or Help Wanted

As unsafe code has been modified a code review from the kernel perspective would be very much appreciated.

Documentation Updated

  • Kernel: The relevant files in /docs have been updated or no updates are required.
  • Userland: The application README has been added, updated, or no updates are required.

Formatting

  • [o] make formatall has been run.

@niklasad1 niklasad1 self-assigned this Feb 5, 2018

@bradjc bradjc added the P-Upkeep label Feb 5, 2018

@alevy

This comment has been minimized.

Show comment
Hide comment
@alevy

alevy Feb 5, 2018

Member

Hrm... This is correct since we set PCNF0 to include a 1-byte S0 field. I think it's reasonable to merge this.

However, there are some confusing issues looking further into the current implementation. Specifically, I believe the on-air length field should be set to 6-bits, not 8 as it is currently set, and s1 should be 2 bits, instead of 0. We are currently saved by the fact that the last 2 bits of the header will always be zero since they are reserved for future use.

Moreover, we're passing up a buffer with a hardware specific structure (i.e. it has 1-byte s0, length and s1 fields instead of packed) to a hardware-agnostic driver. That's... problematic. It might be better to have an associated type in the BLE hil for the packet type with methods for getting at each of the header fields and the payload.

Member

alevy commented Feb 5, 2018

Hrm... This is correct since we set PCNF0 to include a 1-byte S0 field. I think it's reasonable to merge this.

However, there are some confusing issues looking further into the current implementation. Specifically, I believe the on-air length field should be set to 6-bits, not 8 as it is currently set, and s1 should be 2 bits, instead of 0. We are currently saved by the fact that the last 2 bits of the header will always be zero since they are reserved for future use.

Moreover, we're passing up a buffer with a hardware specific structure (i.e. it has 1-byte s0, length and s1 fields instead of packed) to a hardware-agnostic driver. That's... problematic. It might be better to have an associated type in the BLE hil for the packet type with methods for getting at each of the header fields and the payload.

@alevy

alevy approved these changes Feb 5, 2018

@ppannuto

This comment has been minimized.

Show comment
Hide comment
@ppannuto

ppannuto Feb 5, 2018

Member

@torfmaster would you be willing to update with a comment that captures @alevy's explanation of where the +2 comes from?

Member

ppannuto commented Feb 5, 2018

@torfmaster would you be willing to update with a comment that captures @alevy's explanation of where the +2 comes from?

@torfmaster

This comment has been minimized.

Show comment
Hide comment
@torfmaster

torfmaster Feb 6, 2018

Contributor

@ppannuto: You mean a comment in the code?

Contributor

torfmaster commented Feb 6, 2018

@ppannuto: You mean a comment in the code?

@ppannuto

This comment has been minimized.

Show comment
Hide comment
@ppannuto

ppannuto Feb 6, 2018

Member

Yes please -- trying to save a future person the mystery of where a magic +2 came from :)

Member

ppannuto commented Feb 6, 2018

Yes please -- trying to save a future person the mystery of where a magic +2 came from :)

@niklasad1

This comment has been minimized.

Show comment
Hide comment
@niklasad1

niklasad1 Feb 6, 2018

Member

First of all thanks @torfmaster for fixing this bug!

@alevy

Hrm... This is correct since we set PCNF0 to include a 1-byte S0 field. I think it's reasonable to merge this.

However, there are some confusing issues looking further into the current implementation. Specifically, I believe the on-air length field should be set to 6-bits, not 8 as it is currently set, and s1 should be 2 bits, instead of 0. We are currently saved by the fact that the last 2 bits of the header will always be zero since they are reserved for future use.

Yes, I think I did this because to simply radio buffer management a while back. But still I don't understand this, are you saying that other stacks are including to S1 field and adds another byte in the payload? It is still a pretty ugly way to determine the number of bytes received, i.e, reading the header but because the hardware don't provide anything else I think this is the way to go.

Moreover, we're passing up a buffer with a hardware specific structure (i.e. it has 1-byte s0, length and s1 fields instead of packed) to a hardware-agnostic driver. That's... problematic. It might be better to have an associated type in the BLE hil for the packet type with methods for getting at each of the header fields and the payload.

Create an issue I think we need a RFC inorder to construct a better HIL because it is very brute-force at the moment. I was to before I arrived in Berlin but now I don't have that much over time and motivation.

By packed are you referring to a c style struct that not need be serialized in order to pass it directly underlying chip/DMA?

But still I think we need to specify the HIL should more or less corresponds to the interfaces in the spec, especially clear interfaces between controller and host. This makes is hard to implement new features to stack. Maybe, you and the thesis students can come up with something?

Otherwise, I suspect that we will end-up with two different BLE drivers!

Member

niklasad1 commented Feb 6, 2018

First of all thanks @torfmaster for fixing this bug!

@alevy

Hrm... This is correct since we set PCNF0 to include a 1-byte S0 field. I think it's reasonable to merge this.

However, there are some confusing issues looking further into the current implementation. Specifically, I believe the on-air length field should be set to 6-bits, not 8 as it is currently set, and s1 should be 2 bits, instead of 0. We are currently saved by the fact that the last 2 bits of the header will always be zero since they are reserved for future use.

Yes, I think I did this because to simply radio buffer management a while back. But still I don't understand this, are you saying that other stacks are including to S1 field and adds another byte in the payload? It is still a pretty ugly way to determine the number of bytes received, i.e, reading the header but because the hardware don't provide anything else I think this is the way to go.

Moreover, we're passing up a buffer with a hardware specific structure (i.e. it has 1-byte s0, length and s1 fields instead of packed) to a hardware-agnostic driver. That's... problematic. It might be better to have an associated type in the BLE hil for the packet type with methods for getting at each of the header fields and the payload.

Create an issue I think we need a RFC inorder to construct a better HIL because it is very brute-force at the moment. I was to before I arrived in Berlin but now I don't have that much over time and motivation.

By packed are you referring to a c style struct that not need be serialized in order to pass it directly underlying chip/DMA?

But still I think we need to specify the HIL should more or less corresponds to the interfaces in the spec, especially clear interfaces between controller and host. This makes is hard to implement new features to stack. Maybe, you and the thesis students can come up with something?

Otherwise, I suspect that we will end-up with two different BLE drivers!

@torfmaster

This comment has been minimized.

Show comment
Hide comment
@torfmaster

torfmaster Feb 7, 2018

Contributor

Just to make sure, I got everything right: the length of the data passed to the driver in "receive_event" computes as S0 (1 bvte) + length (1 byte)+ S1 (0 bytes) + payload, right?

Contributor

torfmaster commented Feb 7, 2018

Just to make sure, I got everything right: the length of the data passed to the driver in "receive_event" computes as S0 (1 bvte) + length (1 byte)+ S1 (0 bytes) + payload, right?

@alevy alevy merged commit d153b2e into tock:master Feb 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@alevy

This comment has been minimized.

Show comment
Hide comment
@alevy

alevy Feb 8, 2018

Member

But still I don't understand this, are you saying that other stacks are including to S1 field and adds another byte in the payload? It is still a pretty ugly way to determine the number of bytes received, i.e, reading the header but because the hardware don't provide anything else I think this is the way to go.

So, the S0 and S1 fields are not really meaningful in BLE. They seem to be a way for the Nordic hardware to support both BLE and their own proprietary protocol (which I'm guessing has a different header format) in the same hardware controller.

The BLE header for advertising packets consists of 8 bits for the PDU type and TxAdd/RxAdd fields, followed by a 6-bit length field, followed by two reserved bits (supposed to be 0).

In the hardware, S0, S1 and Length are adjustable length fields. They always show up in memory as a single byte (unless S! is configured to not appear), but they can correspond to different sizes in on-air packets. So, for an advertising packet, you'd really want to configure S0 to be 8-bits, Length to be 6-bits, and S1 to be 0-bits.

But importantly, the memory buffer that's gets filled by the DMA will not contain a packed header, but a separate byte for each included field. It's highly likely that other hardware won't do the same thing, and instead just copy the on-air packet as-is, so it's seems suboptimal to have an interface that relies on this property.

Create an issue I think we need a RFC inorder to construct a better HIL because it is very brute-force at the moment.

Agreed.

Member

alevy commented Feb 8, 2018

But still I don't understand this, are you saying that other stacks are including to S1 field and adds another byte in the payload? It is still a pretty ugly way to determine the number of bytes received, i.e, reading the header but because the hardware don't provide anything else I think this is the way to go.

So, the S0 and S1 fields are not really meaningful in BLE. They seem to be a way for the Nordic hardware to support both BLE and their own proprietary protocol (which I'm guessing has a different header format) in the same hardware controller.

The BLE header for advertising packets consists of 8 bits for the PDU type and TxAdd/RxAdd fields, followed by a 6-bit length field, followed by two reserved bits (supposed to be 0).

In the hardware, S0, S1 and Length are adjustable length fields. They always show up in memory as a single byte (unless S! is configured to not appear), but they can correspond to different sizes in on-air packets. So, for an advertising packet, you'd really want to configure S0 to be 8-bits, Length to be 6-bits, and S1 to be 0-bits.

But importantly, the memory buffer that's gets filled by the DMA will not contain a packed header, but a separate byte for each included field. It's highly likely that other hardware won't do the same thing, and instead just copy the on-air packet as-is, so it's seems suboptimal to have an interface that relies on this property.

Create an issue I think we need a RFC inorder to construct a better HIL because it is very brute-force at the moment.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment