* [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-16 9:21 ` Zefir Kurtisi
0 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-16 9:21 UTC (permalink / raw)
To: linux-wireless; +Cc: Nick Kossifidis, ath9k-devel, Zefir Kurtisi
The max_index value provided in the spectral data set
has to be interpreted differently for HT20 and HT40.
The spectral module initially supported HT20 only
and the spectral_max_index() function handled the
conversion from signed to unsigned.
In HT40, the max_index is an unsigned value and does
not need to be fixed. When HT40 support was added,
the function was exteded to handle the conversion
for HT20 based on the provided num_bins parameter.
This is misleading and complex. Instead this patch
shifts the correction of the value to the caller,
which in effect reduces it to a singel bit-operation
for HT20 data.
Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
drivers/net/wireless/ath/ath9k/common-spectral.c | 20 ++++++-------
drivers/net/wireless/ath/ath9k/common-spectral.h | 36 +++++++-----------------
2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
index a876271..e4bae40 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.c
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
@@ -59,8 +59,8 @@ ath_cmn_max_idx_verify_ht20_fft(u8 *sample_end, int bytes_read)
sample = sample_end - SPECTRAL_HT20_SAMPLE_LEN + 1;
- max_index = spectral_max_index(mag_info->all_bins,
- SPECTRAL_HT20_NUM_BINS);
+ /* in ht20, this is a 6-bit signed number => shift it to 0 */
+ max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
max_magnitude = spectral_max_magnitude(mag_info->all_bins);
max_exp = mag_info->max_exp & 0xf;
@@ -100,12 +100,10 @@ ath_cmn_max_idx_verify_ht20_40_fft(u8 *sample_end, int bytes_read)
sample = sample_end - SPECTRAL_HT20_40_SAMPLE_LEN + 1;
lower_mag = spectral_max_magnitude(mag_info->lower_bins);
- lower_max_index = spectral_max_index(mag_info->lower_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ lower_max_index = spectral_max_index(mag_info->lower_bins);
upper_mag = spectral_max_magnitude(mag_info->upper_bins);
- upper_max_index = spectral_max_index(mag_info->upper_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ upper_max_index = spectral_max_index(mag_info->upper_bins);
max_exp = mag_info->max_exp & 0xf;
@@ -169,8 +167,8 @@ ath_cmn_process_ht20_fft(struct ath_rx_status *rs,
magnitude = spectral_max_magnitude(mag_info->all_bins);
fft_sample_20.max_magnitude = __cpu_to_be16(magnitude);
- max_index = spectral_max_index(mag_info->all_bins,
- SPECTRAL_HT20_NUM_BINS);
+ /* in ht20, this is a 6-bit signed number => shift it to 0 */
+ max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
fft_sample_20.max_index = max_index;
bitmap_w = spectral_bitmap_weight(mag_info->all_bins);
@@ -302,12 +300,10 @@ ath_cmn_process_ht20_40_fft(struct ath_rx_status *rs,
upper_mag = spectral_max_magnitude(mag_info->upper_bins);
fft_sample_40.upper_max_magnitude = __cpu_to_be16(upper_mag);
- lower_max_index = spectral_max_index(mag_info->lower_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ lower_max_index = spectral_max_index(mag_info->lower_bins);
fft_sample_40.lower_max_index = lower_max_index;
- upper_max_index = spectral_max_index(mag_info->upper_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ upper_max_index = spectral_max_index(mag_info->upper_bins);
fft_sample_40.upper_max_index = upper_max_index;
lower_bitmap_w = spectral_bitmap_weight(mag_info->lower_bins);
diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.h b/drivers/net/wireless/ath/ath9k/common-spectral.h
index 998743b..4cc5a6c 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.h
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.h
@@ -116,33 +116,17 @@ static inline u16 spectral_max_magnitude(u8 *bins)
(bins[2] & 0x03) << 10;
}
-/* return the max magnitude from the all/upper/lower bins */
-static inline u8 spectral_max_index(u8 *bins, int num_bins)
+/* return the max magnitude from the all/upper/lower bins
+ *
+ * in HT20: 6-bit signed number of range -28 to +27
+ * in HT40: 6-bit unsigned number of range 0 to +63
+ * (upper sub-channel index 0 is DC)
+ *
+ * Correct interpretation of the value has to be done at caller
+ */
+static inline u8 spectral_max_index(u8 *bins)
{
- s8 m = (bins[2] & 0xfc) >> 2;
- u8 zero_idx = num_bins / 2;
-
- /* It's a 5 bit signed int, remove its sign and use one's
- * complement interpretation to add the sign back to the 8
- * bit int
- */
- if (m & 0x20) {
- m &= ~0x20;
- m |= 0xe0;
- }
-
- /* Bring the zero point to the beginning
- * instead of the middle so that we can use
- * it for array lookup and that we don't deal
- * with negative values later
- */
- m += zero_idx;
-
- /* Sanity check to make sure index is within bounds */
- if (m < 0 || m > num_bins - 1)
- m = 0;
-
- return m;
+ return (bins[2] & 0xfc) >> 2;
}
/* return the bitmap weight from the all/upper/lower bins */
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-16 9:21 ` Zefir Kurtisi
0 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-16 9:21 UTC (permalink / raw)
To: ath9k-devel
The max_index value provided in the spectral data set
has to be interpreted differently for HT20 and HT40.
The spectral module initially supported HT20 only
and the spectral_max_index() function handled the
conversion from signed to unsigned.
In HT40, the max_index is an unsigned value and does
not need to be fixed. When HT40 support was added,
the function was exteded to handle the conversion
for HT20 based on the provided num_bins parameter.
This is misleading and complex. Instead this patch
shifts the correction of the value to the caller,
which in effect reduces it to a singel bit-operation
for HT20 data.
Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
drivers/net/wireless/ath/ath9k/common-spectral.c | 20 ++++++-------
drivers/net/wireless/ath/ath9k/common-spectral.h | 36 +++++++-----------------
2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
index a876271..e4bae40 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.c
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
@@ -59,8 +59,8 @@ ath_cmn_max_idx_verify_ht20_fft(u8 *sample_end, int bytes_read)
sample = sample_end - SPECTRAL_HT20_SAMPLE_LEN + 1;
- max_index = spectral_max_index(mag_info->all_bins,
- SPECTRAL_HT20_NUM_BINS);
+ /* in ht20, this is a 6-bit signed number => shift it to 0 */
+ max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
max_magnitude = spectral_max_magnitude(mag_info->all_bins);
max_exp = mag_info->max_exp & 0xf;
@@ -100,12 +100,10 @@ ath_cmn_max_idx_verify_ht20_40_fft(u8 *sample_end, int bytes_read)
sample = sample_end - SPECTRAL_HT20_40_SAMPLE_LEN + 1;
lower_mag = spectral_max_magnitude(mag_info->lower_bins);
- lower_max_index = spectral_max_index(mag_info->lower_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ lower_max_index = spectral_max_index(mag_info->lower_bins);
upper_mag = spectral_max_magnitude(mag_info->upper_bins);
- upper_max_index = spectral_max_index(mag_info->upper_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ upper_max_index = spectral_max_index(mag_info->upper_bins);
max_exp = mag_info->max_exp & 0xf;
@@ -169,8 +167,8 @@ ath_cmn_process_ht20_fft(struct ath_rx_status *rs,
magnitude = spectral_max_magnitude(mag_info->all_bins);
fft_sample_20.max_magnitude = __cpu_to_be16(magnitude);
- max_index = spectral_max_index(mag_info->all_bins,
- SPECTRAL_HT20_NUM_BINS);
+ /* in ht20, this is a 6-bit signed number => shift it to 0 */
+ max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
fft_sample_20.max_index = max_index;
bitmap_w = spectral_bitmap_weight(mag_info->all_bins);
@@ -302,12 +300,10 @@ ath_cmn_process_ht20_40_fft(struct ath_rx_status *rs,
upper_mag = spectral_max_magnitude(mag_info->upper_bins);
fft_sample_40.upper_max_magnitude = __cpu_to_be16(upper_mag);
- lower_max_index = spectral_max_index(mag_info->lower_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ lower_max_index = spectral_max_index(mag_info->lower_bins);
fft_sample_40.lower_max_index = lower_max_index;
- upper_max_index = spectral_max_index(mag_info->upper_bins,
- SPECTRAL_HT20_40_NUM_BINS);
+ upper_max_index = spectral_max_index(mag_info->upper_bins);
fft_sample_40.upper_max_index = upper_max_index;
lower_bitmap_w = spectral_bitmap_weight(mag_info->lower_bins);
diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.h b/drivers/net/wireless/ath/ath9k/common-spectral.h
index 998743b..4cc5a6c 100644
--- a/drivers/net/wireless/ath/ath9k/common-spectral.h
+++ b/drivers/net/wireless/ath/ath9k/common-spectral.h
@@ -116,33 +116,17 @@ static inline u16 spectral_max_magnitude(u8 *bins)
(bins[2] & 0x03) << 10;
}
-/* return the max magnitude from the all/upper/lower bins */
-static inline u8 spectral_max_index(u8 *bins, int num_bins)
+/* return the max magnitude from the all/upper/lower bins
+ *
+ * in HT20: 6-bit signed number of range -28 to +27
+ * in HT40: 6-bit unsigned number of range 0 to +63
+ * (upper sub-channel index 0 is DC)
+ *
+ * Correct interpretation of the value has to be done@caller
+ */
+static inline u8 spectral_max_index(u8 *bins)
{
- s8 m = (bins[2] & 0xfc) >> 2;
- u8 zero_idx = num_bins / 2;
-
- /* It's a 5 bit signed int, remove its sign and use one's
- * complement interpretation to add the sign back to the 8
- * bit int
- */
- if (m & 0x20) {
- m &= ~0x20;
- m |= 0xe0;
- }
-
- /* Bring the zero point to the beginning
- * instead of the middle so that we can use
- * it for array lookup and that we don't deal
- * with negative values later
- */
- m += zero_idx;
-
- /* Sanity check to make sure index is within bounds */
- if (m < 0 || m > num_bins - 1)
- m = 0;
-
- return m;
+ return (bins[2] & 0xfc) >> 2;
}
/* return the bitmap weight from the all/upper/lower bins */
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-16 9:21 ` [ath9k-devel] " Zefir Kurtisi
@ 2015-06-18 8:43 ` Nick Kossifidis
-1 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 8:43 UTC (permalink / raw)
To: Zefir Kurtisi; +Cc: linux-wireless, ath9k-devel
max_index is a 6bit signed integer in both cases (sorry for the 5bit
typo in the comments), so the current function handles it correctly
for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
Also you don't handle the negative indices we get from the hardware
(you just remove the sign). Have you tested this and if you did on
which hardware did you do the test ?
2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> The max_index value provided in the spectral data set
> has to be interpreted differently for HT20 and HT40.
>
> The spectral module initially supported HT20 only
> and the spectral_max_index() function handled the
> conversion from signed to unsigned.
>
> In HT40, the max_index is an unsigned value and does
> not need to be fixed. When HT40 support was added,
> the function was exteded to handle the conversion
> for HT20 based on the provided num_bins parameter.
>
> This is misleading and complex. Instead this patch
> shifts the correction of the value to the caller,
> which in effect reduces it to a singel bit-operation
> for HT20 data.
>
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> ---
> drivers/net/wireless/ath/ath9k/common-spectral.c | 20 ++++++-------
> drivers/net/wireless/ath/ath9k/common-spectral.h | 36 +++++++-----------------
> 2 files changed, 18 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
> index a876271..e4bae40 100644
> --- a/drivers/net/wireless/ath/ath9k/common-spectral.c
> +++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
> @@ -59,8 +59,8 @@ ath_cmn_max_idx_verify_ht20_fft(u8 *sample_end, int bytes_read)
>
> sample = sample_end - SPECTRAL_HT20_SAMPLE_LEN + 1;
>
> - max_index = spectral_max_index(mag_info->all_bins,
> - SPECTRAL_HT20_NUM_BINS);
> + /* in ht20, this is a 6-bit signed number => shift it to 0 */
> + max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
> max_magnitude = spectral_max_magnitude(mag_info->all_bins);
>
> max_exp = mag_info->max_exp & 0xf;
> @@ -100,12 +100,10 @@ ath_cmn_max_idx_verify_ht20_40_fft(u8 *sample_end, int bytes_read)
> sample = sample_end - SPECTRAL_HT20_40_SAMPLE_LEN + 1;
>
> lower_mag = spectral_max_magnitude(mag_info->lower_bins);
> - lower_max_index = spectral_max_index(mag_info->lower_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + lower_max_index = spectral_max_index(mag_info->lower_bins);
>
> upper_mag = spectral_max_magnitude(mag_info->upper_bins);
> - upper_max_index = spectral_max_index(mag_info->upper_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + upper_max_index = spectral_max_index(mag_info->upper_bins);
>
> max_exp = mag_info->max_exp & 0xf;
>
> @@ -169,8 +167,8 @@ ath_cmn_process_ht20_fft(struct ath_rx_status *rs,
> magnitude = spectral_max_magnitude(mag_info->all_bins);
> fft_sample_20.max_magnitude = __cpu_to_be16(magnitude);
>
> - max_index = spectral_max_index(mag_info->all_bins,
> - SPECTRAL_HT20_NUM_BINS);
> + /* in ht20, this is a 6-bit signed number => shift it to 0 */
> + max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
> fft_sample_20.max_index = max_index;
>
> bitmap_w = spectral_bitmap_weight(mag_info->all_bins);
> @@ -302,12 +300,10 @@ ath_cmn_process_ht20_40_fft(struct ath_rx_status *rs,
> upper_mag = spectral_max_magnitude(mag_info->upper_bins);
> fft_sample_40.upper_max_magnitude = __cpu_to_be16(upper_mag);
>
> - lower_max_index = spectral_max_index(mag_info->lower_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + lower_max_index = spectral_max_index(mag_info->lower_bins);
> fft_sample_40.lower_max_index = lower_max_index;
>
> - upper_max_index = spectral_max_index(mag_info->upper_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + upper_max_index = spectral_max_index(mag_info->upper_bins);
> fft_sample_40.upper_max_index = upper_max_index;
>
> lower_bitmap_w = spectral_bitmap_weight(mag_info->lower_bins);
> diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.h b/drivers/net/wireless/ath/ath9k/common-spectral.h
> index 998743b..4cc5a6c 100644
> --- a/drivers/net/wireless/ath/ath9k/common-spectral.h
> +++ b/drivers/net/wireless/ath/ath9k/common-spectral.h
> @@ -116,33 +116,17 @@ static inline u16 spectral_max_magnitude(u8 *bins)
> (bins[2] & 0x03) << 10;
> }
>
> -/* return the max magnitude from the all/upper/lower bins */
> -static inline u8 spectral_max_index(u8 *bins, int num_bins)
> +/* return the max magnitude from the all/upper/lower bins
> + *
> + * in HT20: 6-bit signed number of range -28 to +27
> + * in HT40: 6-bit unsigned number of range 0 to +63
> + * (upper sub-channel index 0 is DC)
> + *
> + * Correct interpretation of the value has to be done at caller
> + */
> +static inline u8 spectral_max_index(u8 *bins)
> {
> - s8 m = (bins[2] & 0xfc) >> 2;
> - u8 zero_idx = num_bins / 2;
> -
> - /* It's a 5 bit signed int, remove its sign and use one's
> - * complement interpretation to add the sign back to the 8
> - * bit int
> - */
> - if (m & 0x20) {
> - m &= ~0x20;
> - m |= 0xe0;
> - }
> -
> - /* Bring the zero point to the beginning
> - * instead of the middle so that we can use
> - * it for array lookup and that we don't deal
> - * with negative values later
> - */
> - m += zero_idx;
> -
> - /* Sanity check to make sure index is within bounds */
> - if (m < 0 || m > num_bins - 1)
> - m = 0;
> -
> - return m;
> + return (bins[2] & 0xfc) >> 2;
> }
>
> /* return the bitmap weight from the all/upper/lower bins */
> --
> 1.9.1
>
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 8:43 ` Nick Kossifidis
0 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 8:43 UTC (permalink / raw)
To: ath9k-devel
max_index is a 6bit signed integer in both cases (sorry for the 5bit
typo in the comments), so the current function handles it correctly
for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
Also you don't handle the negative indices we get from the hardware
(you just remove the sign). Have you tested this and if you did on
which hardware did you do the test ?
2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> The max_index value provided in the spectral data set
> has to be interpreted differently for HT20 and HT40.
>
> The spectral module initially supported HT20 only
> and the spectral_max_index() function handled the
> conversion from signed to unsigned.
>
> In HT40, the max_index is an unsigned value and does
> not need to be fixed. When HT40 support was added,
> the function was exteded to handle the conversion
> for HT20 based on the provided num_bins parameter.
>
> This is misleading and complex. Instead this patch
> shifts the correction of the value to the caller,
> which in effect reduces it to a singel bit-operation
> for HT20 data.
>
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> ---
> drivers/net/wireless/ath/ath9k/common-spectral.c | 20 ++++++-------
> drivers/net/wireless/ath/ath9k/common-spectral.h | 36 +++++++-----------------
> 2 files changed, 18 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.c b/drivers/net/wireless/ath/ath9k/common-spectral.c
> index a876271..e4bae40 100644
> --- a/drivers/net/wireless/ath/ath9k/common-spectral.c
> +++ b/drivers/net/wireless/ath/ath9k/common-spectral.c
> @@ -59,8 +59,8 @@ ath_cmn_max_idx_verify_ht20_fft(u8 *sample_end, int bytes_read)
>
> sample = sample_end - SPECTRAL_HT20_SAMPLE_LEN + 1;
>
> - max_index = spectral_max_index(mag_info->all_bins,
> - SPECTRAL_HT20_NUM_BINS);
> + /* in ht20, this is a 6-bit signed number => shift it to 0 */
> + max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
> max_magnitude = spectral_max_magnitude(mag_info->all_bins);
>
> max_exp = mag_info->max_exp & 0xf;
> @@ -100,12 +100,10 @@ ath_cmn_max_idx_verify_ht20_40_fft(u8 *sample_end, int bytes_read)
> sample = sample_end - SPECTRAL_HT20_40_SAMPLE_LEN + 1;
>
> lower_mag = spectral_max_magnitude(mag_info->lower_bins);
> - lower_max_index = spectral_max_index(mag_info->lower_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + lower_max_index = spectral_max_index(mag_info->lower_bins);
>
> upper_mag = spectral_max_magnitude(mag_info->upper_bins);
> - upper_max_index = spectral_max_index(mag_info->upper_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + upper_max_index = spectral_max_index(mag_info->upper_bins);
>
> max_exp = mag_info->max_exp & 0xf;
>
> @@ -169,8 +167,8 @@ ath_cmn_process_ht20_fft(struct ath_rx_status *rs,
> magnitude = spectral_max_magnitude(mag_info->all_bins);
> fft_sample_20.max_magnitude = __cpu_to_be16(magnitude);
>
> - max_index = spectral_max_index(mag_info->all_bins,
> - SPECTRAL_HT20_NUM_BINS);
> + /* in ht20, this is a 6-bit signed number => shift it to 0 */
> + max_index = spectral_max_index(mag_info->all_bins) ^ 0x20;
> fft_sample_20.max_index = max_index;
>
> bitmap_w = spectral_bitmap_weight(mag_info->all_bins);
> @@ -302,12 +300,10 @@ ath_cmn_process_ht20_40_fft(struct ath_rx_status *rs,
> upper_mag = spectral_max_magnitude(mag_info->upper_bins);
> fft_sample_40.upper_max_magnitude = __cpu_to_be16(upper_mag);
>
> - lower_max_index = spectral_max_index(mag_info->lower_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + lower_max_index = spectral_max_index(mag_info->lower_bins);
> fft_sample_40.lower_max_index = lower_max_index;
>
> - upper_max_index = spectral_max_index(mag_info->upper_bins,
> - SPECTRAL_HT20_40_NUM_BINS);
> + upper_max_index = spectral_max_index(mag_info->upper_bins);
> fft_sample_40.upper_max_index = upper_max_index;
>
> lower_bitmap_w = spectral_bitmap_weight(mag_info->lower_bins);
> diff --git a/drivers/net/wireless/ath/ath9k/common-spectral.h b/drivers/net/wireless/ath/ath9k/common-spectral.h
> index 998743b..4cc5a6c 100644
> --- a/drivers/net/wireless/ath/ath9k/common-spectral.h
> +++ b/drivers/net/wireless/ath/ath9k/common-spectral.h
> @@ -116,33 +116,17 @@ static inline u16 spectral_max_magnitude(u8 *bins)
> (bins[2] & 0x03) << 10;
> }
>
> -/* return the max magnitude from the all/upper/lower bins */
> -static inline u8 spectral_max_index(u8 *bins, int num_bins)
> +/* return the max magnitude from the all/upper/lower bins
> + *
> + * in HT20: 6-bit signed number of range -28 to +27
> + * in HT40: 6-bit unsigned number of range 0 to +63
> + * (upper sub-channel index 0 is DC)
> + *
> + * Correct interpretation of the value has to be done at caller
> + */
> +static inline u8 spectral_max_index(u8 *bins)
> {
> - s8 m = (bins[2] & 0xfc) >> 2;
> - u8 zero_idx = num_bins / 2;
> -
> - /* It's a 5 bit signed int, remove its sign and use one's
> - * complement interpretation to add the sign back to the 8
> - * bit int
> - */
> - if (m & 0x20) {
> - m &= ~0x20;
> - m |= 0xe0;
> - }
> -
> - /* Bring the zero point to the beginning
> - * instead of the middle so that we can use
> - * it for array lookup and that we don't deal
> - * with negative values later
> - */
> - m += zero_idx;
> -
> - /* Sanity check to make sure index is within bounds */
> - if (m < 0 || m > num_bins - 1)
> - m = 0;
> -
> - return m;
> + return (bins[2] & 0xfc) >> 2;
> }
>
> /* return the bitmap weight from the all/upper/lower bins */
> --
> 1.9.1
>
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 8:43 ` [ath9k-devel] " Nick Kossifidis
@ 2015-06-18 10:36 ` Zefir Kurtisi
-1 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 10:36 UTC (permalink / raw)
To: Nick Kossifidis; +Cc: linux-wireless, ath9k-devel
On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
> max_index is a 6bit signed integer in both cases (sorry for the 5bit
> typo in the comments), so the current function handles it correctly
> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
> Also you don't handle the negative indices we get from the hardware
> (you just remove the sign). Have you tested this and if you did on
> which hardware did you do the test ?
>
> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>> [...]
>> +/* return the max magnitude from the all/upper/lower bins
>> + *
>> + * in HT20: 6-bit signed number of range -28 to +27
>> + * in HT40: 6-bit unsigned number of range 0 to +63
>> + * (upper sub-channel index 0 is DC)
>> + *
>> + * Correct interpretation of the value has to be done at caller
>> + */
The comment above is taken from developer NDA documents and should be correct.
With that, in HT40 mode the max_index value has to be taken as is, while in HT20
it needs to be shifted to the unsigned range.
I used the proposed method with the chirp detector for FFTs provided for long
radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
the same way as with spectral, but now I realize my mistake: for chirp detection,
the relative max_index is sufficient, while for spectral the absolute value is needed.
Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
an offset of 4, therefore the correct operation should be:
ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
The following code snipped proves the correctness of the operation:
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
const char *char_to_bin(char x)
{
int z;
static char b[7];
b[0] = '\0';
for (z = 32; z > 0; z >>= 1)
strcat(b, ((x & z) == z) ? "1" : "0");
return b;
}
int main (void)
{
char s;
for (s = -28; s < 28; s++)
printf("%3d: %s\n", s, char_to_bin((s ^ 0x20) - 4));
return 0;
}
I'll provide a v2 patch for you to test. My intention with this is not to fix
something that is already working, but to share the related functions between
spectral and chirp detector.
Thanks,
Zefir
^ permalink raw reply [flat|nested] 18+ messages in thread* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 10:36 ` Zefir Kurtisi
0 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 10:36 UTC (permalink / raw)
To: ath9k-devel
On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
> max_index is a 6bit signed integer in both cases (sorry for the 5bit
> typo in the comments), so the current function handles it correctly
> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
> Also you don't handle the negative indices we get from the hardware
> (you just remove the sign). Have you tested this and if you did on
> which hardware did you do the test ?
>
> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>> [...]
>> +/* return the max magnitude from the all/upper/lower bins
>> + *
>> + * in HT20: 6-bit signed number of range -28 to +27
>> + * in HT40: 6-bit unsigned number of range 0 to +63
>> + * (upper sub-channel index 0 is DC)
>> + *
>> + * Correct interpretation of the value has to be done at caller
>> + */
The comment above is taken from developer NDA documents and should be correct.
With that, in HT40 mode the max_index value has to be taken as is, while in HT20
it needs to be shifted to the unsigned range.
I used the proposed method with the chirp detector for FFTs provided for long
radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
the same way as with spectral, but now I realize my mistake: for chirp detection,
the relative max_index is sufficient, while for spectral the absolute value is needed.
Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
an offset of 4, therefore the correct operation should be:
ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
The following code snipped proves the correctness of the operation:
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
const char *char_to_bin(char x)
{
int z;
static char b[7];
b[0] = '\0';
for (z = 32; z > 0; z >>= 1)
strcat(b, ((x & z) == z) ? "1" : "0");
return b;
}
int main (void)
{
char s;
for (s = -28; s < 28; s++)
printf("%3d: %s\n", s, char_to_bin((s ^ 0x20) - 4));
return 0;
}
I'll provide a v2 patch for you to test. My intention with this is not to fix
something that is already working, but to share the related functions between
spectral and chirp detector.
Thanks,
Zefir
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 10:36 ` [ath9k-devel] " Zefir Kurtisi
@ 2015-06-18 14:13 ` Nick Kossifidis
-1 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 14:13 UTC (permalink / raw)
To: Zefir Kurtisi; +Cc: linux-wireless, ath9k-devel
2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>> typo in the comments), so the current function handles it correctly
>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>> Also you don't handle the negative indices we get from the hardware
>> (you just remove the sign). Have you tested this and if you did on
>> which hardware did you do the test ?
>>
>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>> [...]
>>> +/* return the max magnitude from the all/upper/lower bins
>>> + *
>>> + * in HT20: 6-bit signed number of range -28 to +27
>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>> + * (upper sub-channel index 0 is DC)
>>> + *
>>> + * Correct interpretation of the value has to be done at caller
>>> + */
>
> The comment above is taken from developer NDA documents and should be correct.
> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
> it needs to be shifted to the unsigned range.
>
I have NDA documents as well stating that the indices are from -64 to
63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
that we get 128bins on dynamic HT20/40, see the header files too:
#define SPECTRAL_HT20_40_NUM_BINS 128
and/or the received packet length. Maybe you are talking about
"static" HT40 (I don't see anything about that on the documents I
have) or something else.
> I used the proposed method with the chirp detector for FFTs provided for long
> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
> the same way as with spectral, but now I realize my mistake: for chirp detection,
> the relative max_index is sufficient, while for spectral the absolute value is needed.
>
> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
> an offset of 4, therefore the correct operation should be:
> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>
Have in mind that on earlier chips (I did the testing on an AR9820) we
get corrupted frames sometimes so we also need the sanity check I put
there or else we can end up reading data out of bounds which is pretty
dangerous so please leave the current implementation there as is.
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 14:13 ` Nick Kossifidis
0 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 14:13 UTC (permalink / raw)
To: ath9k-devel
2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>> typo in the comments), so the current function handles it correctly
>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>> Also you don't handle the negative indices we get from the hardware
>> (you just remove the sign). Have you tested this and if you did on
>> which hardware did you do the test ?
>>
>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>> [...]
>>> +/* return the max magnitude from the all/upper/lower bins
>>> + *
>>> + * in HT20: 6-bit signed number of range -28 to +27
>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>> + * (upper sub-channel index 0 is DC)
>>> + *
>>> + * Correct interpretation of the value has to be done at caller
>>> + */
>
> The comment above is taken from developer NDA documents and should be correct.
> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
> it needs to be shifted to the unsigned range.
>
I have NDA documents as well stating that the indices are from -64 to
63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
that we get 128bins on dynamic HT20/40, see the header files too:
#define SPECTRAL_HT20_40_NUM_BINS 128
and/or the received packet length. Maybe you are talking about
"static" HT40 (I don't see anything about that on the documents I
have) or something else.
> I used the proposed method with the chirp detector for FFTs provided for long
> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
> the same way as with spectral, but now I realize my mistake: for chirp detection,
> the relative max_index is sufficient, while for spectral the absolute value is needed.
>
> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
> an offset of 4, therefore the correct operation should be:
> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>
Have in mind that on earlier chips (I did the testing on an AR9820) we
get corrupted frames sometimes so we also need the sanity check I put
there or else we can end up reading data out of bounds which is pretty
dangerous so please leave the current implementation there as is.
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 14:13 ` [ath9k-devel] " Nick Kossifidis
@ 2015-06-18 15:11 ` Zefir Kurtisi
-1 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 15:11 UTC (permalink / raw)
To: Nick Kossifidis; +Cc: linux-wireless, ath9k-devel
On 06/18/2015 04:13 PM, Nick Kossifidis wrote:
> [...]
> I have NDA documents as well stating that the indices are from -64 to
> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
> that we get 128bins on dynamic HT20/40, see the header files too:
>
> #define SPECTRAL_HT20_40_NUM_BINS 128
>
Right, there are 128 in total, combined from 2 individual 64-bin sets. That's why
you need to swap upper and lower when operating in HT40MINUS, since chip always
puts primary channel to lower bin set.
> and/or the received packet length. Maybe you are talking about
> "static" HT40 (I don't see anything about that on the documents I
> have) or something else.
>
Yes, maybe different documents. Mine says that interpretation of upper/lower bin
values (max_magnitude, bitmap_weight, max_index) is the same in static and dynamic
modes - only the bin values differ.
>> I used the proposed method with the chirp detector for FFTs provided for long
>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>
>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>> an offset of 4, therefore the correct operation should be:
>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>
>
> Have in mind that on earlier chips (I did the testing on an AR9820) we
> get corrupted frames sometimes so we also need the sanity check I put
> there or else we can end up reading data out of bounds which is pretty
> dangerous so please leave the current implementation there as is.
>
>
The sanity checks / fixes are done before you look at the bin attributes and,
which therefore is not relevant for interpreting them.
But never mind - as said before, I proposed this to use the 3 functions for DFS
chirp detection. I'm fine not touching the running system and instead keep a local
copy of those.
Thanks,
Zefir
^ permalink raw reply [flat|nested] 18+ messages in thread
* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 15:11 ` Zefir Kurtisi
0 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 15:11 UTC (permalink / raw)
To: ath9k-devel
On 06/18/2015 04:13 PM, Nick Kossifidis wrote:
> [...]
> I have NDA documents as well stating that the indices are from -64 to
> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
> that we get 128bins on dynamic HT20/40, see the header files too:
>
> #define SPECTRAL_HT20_40_NUM_BINS 128
>
Right, there are 128 in total, combined from 2 individual 64-bin sets. That's why
you need to swap upper and lower when operating in HT40MINUS, since chip always
puts primary channel to lower bin set.
> and/or the received packet length. Maybe you are talking about
> "static" HT40 (I don't see anything about that on the documents I
> have) or something else.
>
Yes, maybe different documents. Mine says that interpretation of upper/lower bin
values (max_magnitude, bitmap_weight, max_index) is the same in static and dynamic
modes - only the bin values differ.
>> I used the proposed method with the chirp detector for FFTs provided for long
>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>
>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>> an offset of 4, therefore the correct operation should be:
>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>
>
> Have in mind that on earlier chips (I did the testing on an AR9820) we
> get corrupted frames sometimes so we also need the sanity check I put
> there or else we can end up reading data out of bounds which is pretty
> dangerous so please leave the current implementation there as is.
>
>
The sanity checks / fixes are done before you look at the bin attributes and,
which therefore is not relevant for interpreting them.
But never mind - as said before, I proposed this to use the 3 functions for DFS
chirp detection. I'm fine not touching the running system and instead keep a local
copy of those.
Thanks,
Zefir
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 15:11 ` [ath9k-devel] " Zefir Kurtisi
@ 2015-06-18 15:59 ` Nick Kossifidis
-1 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:59 UTC (permalink / raw)
To: Zefir Kurtisi; +Cc: linux-wireless, ath9k-devel
2015-06-18 17:11 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> On 06/18/2015 04:13 PM, Nick Kossifidis wrote:
>> [...]
>> I have NDA documents as well stating that the indices are from -64 to
>> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
>> that we get 128bins on dynamic HT20/40, see the header files too:
>>
>> #define SPECTRAL_HT20_40_NUM_BINS 128
>>
> Right, there are 128 in total, combined from 2 individual 64-bin sets. That's why
> you need to swap upper and lower when operating in HT40MINUS, since chip always
> puts primary channel to lower bin set.
>
>> and/or the received packet length. Maybe you are talking about
>> "static" HT40 (I don't see anything about that on the documents I
>> have) or something else.
>>
> Yes, maybe different documents. Mine says that interpretation of upper/lower bin
> values (max_magnitude, bitmap_weight, max_index) is the same in static and dynamic
> modes - only the bin values differ.
>
>>> I used the proposed method with the chirp detector for FFTs provided for long
>>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>>
>>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>>> an offset of 4, therefore the correct operation should be:
>>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>>
>>
>> Have in mind that on earlier chips (I did the testing on an AR9820) we
>> get corrupted frames sometimes so we also need the sanity check I put
>> there or else we can end up reading data out of bounds which is pretty
>> dangerous so please leave the current implementation there as is.
>>
>>
> The sanity checks / fixes are done before you look at the bin attributes and,
> which therefore is not relevant for interpreting them.
>
Only the lower boundary is checked (in case we are missing a byte -and
we assume it's the first sample-), if we get a corrupted max_idx
that's higher than 64 or 128 and we don't do the check on
spectral_max_index, we 'll have issues when accessing the samples
array to do the magnitude verification.
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 15:59 ` Nick Kossifidis
0 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:59 UTC (permalink / raw)
To: ath9k-devel
2015-06-18 17:11 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
> On 06/18/2015 04:13 PM, Nick Kossifidis wrote:
>> [...]
>> I have NDA documents as well stating that the indices are from -64 to
>> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
>> that we get 128bins on dynamic HT20/40, see the header files too:
>>
>> #define SPECTRAL_HT20_40_NUM_BINS 128
>>
> Right, there are 128 in total, combined from 2 individual 64-bin sets. That's why
> you need to swap upper and lower when operating in HT40MINUS, since chip always
> puts primary channel to lower bin set.
>
>> and/or the received packet length. Maybe you are talking about
>> "static" HT40 (I don't see anything about that on the documents I
>> have) or something else.
>>
> Yes, maybe different documents. Mine says that interpretation of upper/lower bin
> values (max_magnitude, bitmap_weight, max_index) is the same in static and dynamic
> modes - only the bin values differ.
>
>>> I used the proposed method with the chirp detector for FFTs provided for long
>>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>>
>>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>>> an offset of 4, therefore the correct operation should be:
>>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>>
>>
>> Have in mind that on earlier chips (I did the testing on an AR9820) we
>> get corrupted frames sometimes so we also need the sanity check I put
>> there or else we can end up reading data out of bounds which is pretty
>> dangerous so please leave the current implementation there as is.
>>
>>
> The sanity checks / fixes are done before you look at the bin attributes and,
> which therefore is not relevant for interpreting them.
>
Only the lower boundary is checked (in case we are missing a byte -and
we assume it's the first sample-), if we get a corrupted max_idx
that's higher than 64 or 128 and we don't do the check on
spectral_max_index, we 'll have issues when accessing the samples
array to do the magnitude verification.
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 14:13 ` [ath9k-devel] " Nick Kossifidis
@ 2015-06-18 15:34 ` Nick Kossifidis
-1 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:34 UTC (permalink / raw)
To: Zefir Kurtisi; +Cc: linux-wireless, ath9k-devel
2015-06-18 16:13 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
> 2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>>> typo in the comments), so the current function handles it correctly
>>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>>> Also you don't handle the negative indices we get from the hardware
>>> (you just remove the sign). Have you tested this and if you did on
>>> which hardware did you do the test ?
>>>
>>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>>> [...]
>>>> +/* return the max magnitude from the all/upper/lower bins
>>>> + *
>>>> + * in HT20: 6-bit signed number of range -28 to +27
>>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>>> + * (upper sub-channel index 0 is DC)
>>>> + *
>>>> + * Correct interpretation of the value has to be done at caller
>>>> + */
>>
>> The comment above is taken from developer NDA documents and should be correct.
>> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
>> it needs to be shifted to the unsigned range.
>>
>
> I have NDA documents as well stating that the indices are from -64 to
> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
> that we get 128bins on dynamic HT20/40, see the header files too:
>
> #define SPECTRAL_HT20_40_NUM_BINS 128
>
> and/or the received packet length. Maybe you are talking about
> "static" HT40 (I don't see anything about that on the documents I
> have) or something else.
>
To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
for upper bins and since we want an array index of 0 - 128 we add the
index of 0 to the upper max_idx (on the caller). You are right that
the current comment there is wrong (it even mentions 5bit ints) so
feel free to fix that but the code works as expected and it's much
more readable than doing "^ 0x20 - 4" on the caller (plus it handles
both signed and unsigned cases so no problem there).
>> I used the proposed method with the chirp detector for FFTs provided for long
>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>
>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>> an offset of 4, therefore the correct operation should be:
>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>
>
> Have in mind that on earlier chips (I did the testing on an AR9820) we
> get corrupted frames sometimes so we also need the sanity check I put
> there or else we can end up reading data out of bounds which is pretty
> dangerous so please leave the current implementation there as is.
>
A bit more infos here:
On AR9280 there are various issues (check out spectral.c to get an
idea) but I guess they got fixed on later chips so you probably won't
see "shifted" indices etc on AR9590. However both the spectral scan
and your work on chirp detection should work on earlier chips too.
Unfortunately I can't test AR9280's radar detection code because I
have a USB card (so it's ath9k_htc) and there is no DFS support there
yet, but I suggest you test it on an older card to verify that you
won't get any corruption. The good thing is that because you get only
one report (if I remember correctly) in case of radar detection (so
not software triggered spectral scan) you can easily fix that in your
case by checking the packet's length (check out spectral.c for
ath_cmn_copy_fft_frame). Since the format is pretty much the same
(only the magnitude calculation is different), maybe we could handle
the two cases with common code instead (and fix any corruption there).
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 15:34 ` Nick Kossifidis
0 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:34 UTC (permalink / raw)
To: ath9k-devel
2015-06-18 16:13 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
> 2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>>> typo in the comments), so the current function handles it correctly
>>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>>> Also you don't handle the negative indices we get from the hardware
>>> (you just remove the sign). Have you tested this and if you did on
>>> which hardware did you do the test ?
>>>
>>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>>> [...]
>>>> +/* return the max magnitude from the all/upper/lower bins
>>>> + *
>>>> + * in HT20: 6-bit signed number of range -28 to +27
>>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>>> + * (upper sub-channel index 0 is DC)
>>>> + *
>>>> + * Correct interpretation of the value has to be done at caller
>>>> + */
>>
>> The comment above is taken from developer NDA documents and should be correct.
>> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
>> it needs to be shifted to the unsigned range.
>>
>
> I have NDA documents as well stating that the indices are from -64 to
> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
> that we get 128bins on dynamic HT20/40, see the header files too:
>
> #define SPECTRAL_HT20_40_NUM_BINS 128
>
> and/or the received packet length. Maybe you are talking about
> "static" HT40 (I don't see anything about that on the documents I
> have) or something else.
>
To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
for upper bins and since we want an array index of 0 - 128 we add the
index of 0 to the upper max_idx (on the caller). You are right that
the current comment there is wrong (it even mentions 5bit ints) so
feel free to fix that but the code works as expected and it's much
more readable than doing "^ 0x20 - 4" on the caller (plus it handles
both signed and unsigned cases so no problem there).
>> I used the proposed method with the chirp detector for FFTs provided for long
>> radar pulses on an AR9590 (patch posted the same day). Max bin index is used there
>> the same way as with spectral, but now I realize my mistake: for chirp detection,
>> the relative max_index is sufficient, while for spectral the absolute value is needed.
>>
>> Toggling the MSB in HT20 shifts the signed values by 32 and leaves the index with
>> an offset of 4, therefore the correct operation should be:
>> ht20_max_index_absolute = (ht20_max_index ^ 0x20) - 4
>>
>
> Have in mind that on earlier chips (I did the testing on an AR9820) we
> get corrupted frames sometimes so we also need the sanity check I put
> there or else we can end up reading data out of bounds which is pretty
> dangerous so please leave the current implementation there as is.
>
A bit more infos here:
On AR9280 there are various issues (check out spectral.c to get an
idea) but I guess they got fixed on later chips so you probably won't
see "shifted" indices etc on AR9590. However both the spectral scan
and your work on chirp detection should work on earlier chips too.
Unfortunately I can't test AR9280's radar detection code because I
have a USB card (so it's ath9k_htc) and there is no DFS support there
yet, but I suggest you test it on an older card to verify that you
won't get any corruption. The good thing is that because you get only
one report (if I remember correctly) in case of radar detection (so
not software triggered spectral scan) you can easily fix that in your
case by checking the packet's length (check out spectral.c for
ath_cmn_copy_fft_frame). Since the format is pretty much the same
(only the magnitude calculation is different), maybe we could handle
the two cases with common code instead (and fix any corruption there).
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 15:34 ` [ath9k-devel] " Nick Kossifidis
@ 2015-06-18 15:46 ` Nick Kossifidis
-1 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:46 UTC (permalink / raw)
To: Zefir Kurtisi; +Cc: linux-wireless, ath9k-devel
2015-06-18 17:34 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
> 2015-06-18 16:13 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
>> 2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>>>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>>>> typo in the comments), so the current function handles it correctly
>>>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>>>> Also you don't handle the negative indices we get from the hardware
>>>> (you just remove the sign). Have you tested this and if you did on
>>>> which hardware did you do the test ?
>>>>
>>>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>>>> [...]
>>>>> +/* return the max magnitude from the all/upper/lower bins
>>>>> + *
>>>>> + * in HT20: 6-bit signed number of range -28 to +27
>>>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>>>> + * (upper sub-channel index 0 is DC)
>>>>> + *
>>>>> + * Correct interpretation of the value has to be done at caller
>>>>> + */
>>>
>>> The comment above is taken from developer NDA documents and should be correct.
>>> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
>>> it needs to be shifted to the unsigned range.
>>>
>>
>> I have NDA documents as well stating that the indices are from -64 to
>> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
>> that we get 128bins on dynamic HT20/40, see the header files too:
>>
>> #define SPECTRAL_HT20_40_NUM_BINS 128
>>
>> and/or the received packet length. Maybe you are talking about
>> "static" HT40 (I don't see anything about that on the documents I
>> have) or something else.
>>
>
> To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
> for upper bins and since we want an array index of 0 - 128 we add the
> index of 0 to the upper max_idx (on the caller). You are right that
> the current comment there is wrong (it even mentions 5bit ints) so
> feel free to fix that but the code works as expected and it's much
> more readable than doing "^ 0x20 - 4" on the caller (plus it handles
> both signed and unsigned cases so no problem there).
>
Arghh, I need to sleep asap :P
0 - 64 for lower bins and 0 - 63 for upper bins...
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 15:46 ` Nick Kossifidis
0 siblings, 0 replies; 18+ messages in thread
From: Nick Kossifidis @ 2015-06-18 15:46 UTC (permalink / raw)
To: ath9k-devel
2015-06-18 17:34 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
> 2015-06-18 16:13 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
>> 2015-06-18 12:36 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>> On 06/18/2015 10:43 AM, Nick Kossifidis wrote:
>>>> max_index is a 6bit signed integer in both cases (sorry for the 5bit
>>>> typo in the comments), so the current function handles it correctly
>>>> for both HT20 and dynamic HT20/40 modes (I've tested it extensively).
>>>> Also you don't handle the negative indices we get from the hardware
>>>> (you just remove the sign). Have you tested this and if you did on
>>>> which hardware did you do the test ?
>>>>
>>>> 2015-06-16 11:21 GMT+02:00 Zefir Kurtisi <zefir.kurtisi@neratec.com>:
>>>>> [...]
>>>>> +/* return the max magnitude from the all/upper/lower bins
>>>>> + *
>>>>> + * in HT20: 6-bit signed number of range -28 to +27
>>>>> + * in HT40: 6-bit unsigned number of range 0 to +63
>>>>> + * (upper sub-channel index 0 is DC)
>>>>> + *
>>>>> + * Correct interpretation of the value has to be done at caller
>>>>> + */
>>>
>>> The comment above is taken from developer NDA documents and should be correct.
>>> With that, in HT40 mode the max_index value has to be taken as is, while in HT20
>>> it needs to be shifted to the unsigned range.
>>>
>>
>> I have NDA documents as well stating that the indices are from -64 to
>> 63 (-64 to -1, 1 to 63 and 0 is DC), you can check out for yourself
>> that we get 128bins on dynamic HT20/40, see the header files too:
>>
>> #define SPECTRAL_HT20_40_NUM_BINS 128
>>
>> and/or the received packet length. Maybe you are talking about
>> "static" HT40 (I don't see anything about that on the documents I
>> have) or something else.
>>
>
> To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
> for upper bins and since we want an array index of 0 - 128 we add the
> index of 0 to the upper max_idx (on the caller). You are right that
> the current comment there is wrong (it even mentions 5bit ints) so
> feel free to fix that but the code works as expected and it's much
> more readable than doing "^ 0x20 - 4" on the caller (plus it handles
> both signed and unsigned cases so no problem there).
>
Arghh, I need to sleep asap :P
0 - 64 for lower bins and 0 - 63 for upper bins...
--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] ath9k: spectral - simplify max_index calculation
2015-06-18 15:46 ` [ath9k-devel] " Nick Kossifidis
@ 2015-06-18 16:40 ` Zefir Kurtisi
-1 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 16:40 UTC (permalink / raw)
To: Nick Kossifidis; +Cc: linux-wireless, ath9k-devel
On 06/18/2015 05:46 PM, Nick Kossifidis wrote:
> 2015-06-18 17:34 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
>> To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
>> for upper bins and since we want an array index of 0 - 128 we add the
>> index of 0 to the upper max_idx (on the caller). You are right that
>> the current comment there is wrong (it even mentions 5bit ints) so
>> feel free to fix that but the code works as expected and it's much
>> more readable than doing "^ 0x20 - 4" on the caller (plus it handles
>> both signed and unsigned cases so no problem there).
>>
>
> Arghh, I need to sleep asap :P
>
> 0 - 64 for lower bins and 0 - 63 for upper bins...
>
You sure? This would make 129 bins, no ;)
As for your other point:
> A bit more infos here:
>
> On AR9280 there are various issues (check out spectral.c to get an
> idea) but I guess they got fixed on later chips so you probably won't
> see "shifted" indices etc on AR9590. However both the spectral scan
> and your work on chirp detection should work on earlier chips too.
> Unfortunately I can't test AR9280's radar detection code because I
> have a USB card (so it's ath9k_htc) and there is no DFS support there
> yet, but I suggest you test it on an older card to verify that you
> won't get any corruption. The good thing is that because you get only
> one report (if I remember correctly) in case of radar detection (so
> not software triggered spectral scan) you can easily fix that in your
> case by checking the packet's length (check out spectral.c for
> ath_cmn_copy_fft_frame). Since the format is pretty much the same
> (only the magnitude calculation is different), maybe we could handle
> the two cases with common code instead (and fix any corruption there).
There is an important difference between the FFT data provided for spectral and
that for long radar pulses: with the former you always get complete samples, while
the latter can be truncated if the pulse goes away before the analysis is done.
With that, the corrective measures you can perform on spectral data based on the
expected data length and the known bugs causing an invalid length of {-1, +2,
combination thereof} can not be applied for the radar FFT data.
In the patch I posted there is a correction for cases when chip adds 2 extra bytes
before the FFT data (which we observed rarely on AR9590), but I might consider
removing it since it collides with cases where 2 bytes are appended to the data as
part of incomplete sample.
If there is enough interest and people able to test, I agree it would make sense
to generalize and share the code.
Good Night
^ permalink raw reply [flat|nested] 18+ messages in thread* [ath9k-devel] [PATCH] ath9k: spectral - simplify max_index calculation
@ 2015-06-18 16:40 ` Zefir Kurtisi
0 siblings, 0 replies; 18+ messages in thread
From: Zefir Kurtisi @ 2015-06-18 16:40 UTC (permalink / raw)
To: ath9k-devel
On 06/18/2015 05:46 PM, Nick Kossifidis wrote:
> 2015-06-18 17:34 GMT+02:00 Nick Kossifidis <mickflemm@gmail.com>:
>> To clarify this a bit: It's 0 - 63 for lower bins and 0 - 64 (not 63)
>> for upper bins and since we want an array index of 0 - 128 we add the
>> index of 0 to the upper max_idx (on the caller). You are right that
>> the current comment there is wrong (it even mentions 5bit ints) so
>> feel free to fix that but the code works as expected and it's much
>> more readable than doing "^ 0x20 - 4" on the caller (plus it handles
>> both signed and unsigned cases so no problem there).
>>
>
> Arghh, I need to sleep asap :P
>
> 0 - 64 for lower bins and 0 - 63 for upper bins...
>
You sure? This would make 129 bins, no ;)
As for your other point:
> A bit more infos here:
>
> On AR9280 there are various issues (check out spectral.c to get an
> idea) but I guess they got fixed on later chips so you probably won't
> see "shifted" indices etc on AR9590. However both the spectral scan
> and your work on chirp detection should work on earlier chips too.
> Unfortunately I can't test AR9280's radar detection code because I
> have a USB card (so it's ath9k_htc) and there is no DFS support there
> yet, but I suggest you test it on an older card to verify that you
> won't get any corruption. The good thing is that because you get only
> one report (if I remember correctly) in case of radar detection (so
> not software triggered spectral scan) you can easily fix that in your
> case by checking the packet's length (check out spectral.c for
> ath_cmn_copy_fft_frame). Since the format is pretty much the same
> (only the magnitude calculation is different), maybe we could handle
> the two cases with common code instead (and fix any corruption there).
There is an important difference between the FFT data provided for spectral and
that for long radar pulses: with the former you always get complete samples, while
the latter can be truncated if the pulse goes away before the analysis is done.
With that, the corrective measures you can perform on spectral data based on the
expected data length and the known bugs causing an invalid length of {-1, +2,
combination thereof} can not be applied for the radar FFT data.
In the patch I posted there is a correction for cases when chip adds 2 extra bytes
before the FFT data (which we observed rarely on AR9590), but I might consider
removing it since it collides with cases where 2 bytes are appended to the data as
part of incomplete sample.
If there is enough interest and people able to test, I agree it would make sense
to generalize and share the code.
Good Night
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-06-18 16:40 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-16 9:21 [PATCH] ath9k: spectral - simplify max_index calculation Zefir Kurtisi
2015-06-16 9:21 ` [ath9k-devel] " Zefir Kurtisi
2015-06-18 8:43 ` Nick Kossifidis
2015-06-18 8:43 ` [ath9k-devel] " Nick Kossifidis
2015-06-18 10:36 ` Zefir Kurtisi
2015-06-18 10:36 ` [ath9k-devel] " Zefir Kurtisi
2015-06-18 14:13 ` Nick Kossifidis
2015-06-18 14:13 ` [ath9k-devel] " Nick Kossifidis
2015-06-18 15:11 ` Zefir Kurtisi
2015-06-18 15:11 ` [ath9k-devel] " Zefir Kurtisi
2015-06-18 15:59 ` Nick Kossifidis
2015-06-18 15:59 ` [ath9k-devel] " Nick Kossifidis
2015-06-18 15:34 ` Nick Kossifidis
2015-06-18 15:34 ` [ath9k-devel] " Nick Kossifidis
2015-06-18 15:46 ` Nick Kossifidis
2015-06-18 15:46 ` [ath9k-devel] " Nick Kossifidis
2015-06-18 16:40 ` Zefir Kurtisi
2015-06-18 16:40 ` [ath9k-devel] " Zefir Kurtisi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.