PRI value is used as divider when DFS detector analyzes candidate
radar pulses.
If PRI deviation is big from its origin PRI, DFS detector could miss
valid radar reports since HW often misses detecting radar pulses and
causes long interval value of pulses.
For instance from practical results, if runtime PRI is calculated as
1431 for fixed PRI value of 1428 and delta timestamp logs 15719,
the modular remainder will be 1409 and the delta between the remainder
and runtime PRI is 22 that is bigger than PRI tolerance which is 16.
As a result this radar report will be ignored even though it's valid.
By using spec defined PRI for fixed PRI, we can correct this error.
Signed-off-by: Peter Oh <[email protected]>
---
drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c b/drivers/net/wireless/ath/dfs_pri_detector.c
index 1b5ad19..fef459a 100644
--- a/drivers/net/wireless/ath/dfs_pri_detector.c
+++ b/drivers/net/wireless/ath/dfs_pri_detector.c
@@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {};
#define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++)
#define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--)
+#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \
+ (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME)
/**
* struct pulse_elem - elements in pulse queue
@@ -243,7 +245,8 @@ static bool pseq_handler_create_sequences(struct pri_detector *pde,
ps.count_falses = 0;
ps.first_ts = p->ts;
ps.last_ts = ts;
- ps.pri = ts - p->ts;
+ ps.pri = GET_PRI_TO_USE(pde->rs->pri_min,
+ pde->rs->pri_max, ts - p->ts);
ps.dur = ps.pri * (pde->rs->ppb - 1)
+ 2 * pde->rs->max_pri_tolerance;
--
1.9.1
On 03/30/2015 02:55 AM, Zefir Kurtisi wrote:
> On 03/27/2015 05:59 PM, Peter Oh wrote:
>> PRI value is used as divider when DFS detector analyzes candidate
>> radar pulses.
>> If PRI deviation is big from its origin PRI, DFS detector could miss
>> valid radar reports since HW often misses detecting radar pulses and
>> causes long interval value of pulses.
>>
>> For instance from practical results, if runtime PRI is calculated as
>> 1431 for fixed PRI value of 1428 and delta timestamp logs 15719,
>> the modular remainder will be 1409 and the delta between the remainder
>> and runtime PRI is 22 that is bigger than PRI tolerance which is 16.
>> As a result this radar report will be ignored even though it's valid.
>>
>> By using spec defined PRI for fixed PRI, we can correct this error.
>>
> Hm, not sure if this is a generic solution or 10k specific.
This concept is generic, but applies to 9k and 10k only since these 2
drivers are only using the DFS detector.
> As stated in the source code
> /*
> * tolerated deviation of radar time stamp in usecs on both sides
> * TODO: this might need to be HW-dependent
> */
> #define PRI_TOLERANCE 16
>
> the pri tolerance is assumed to be potentially dependent on used HW. The
> comment
> in fact was noted when the detector was written to become generally usable
> by any
> HW out there, before it became Atheros specific.
>
> From my measurements, the ath9k is far within the given pri tolerance,
> i.e. if you
> continuously feed ETSI pattern 0 over a signal generator, it reports PRIs
> very
> accurately at 1428us (or multiples thereof). Even under heavy load the
> jitter of
> those reported values is within +/- 3us. With that, the ath9k (9580 more
> specifically) is already more accurate than the given tolerance -
> increasing it
> further would harm its operability.
as you addressed, current PRI_TOLERANCE value 16 is big enough to cover
both of 9k and 10k.
the biggest offset of PRI on 10k so far I observed is 9us.
Here I'm not increasing PRI tolerance, but using more accurate PRI.
> The pri tolerance does not only help fine-tuning detection performance,
> but also
> heavily defines whether DFS channels are usable or not (afair, it is the
> most
> significant parameter to reduce false positives).
the calculated PRI(ps.pri) at pseq_handler_create_sequences() is used to
filter candidate radar pulses in pde_get_multiple() and
to get total pulse duration.
This change gives more reliable results in filtering candidate radar
pulses and does not give defect on calculating total pulse duration
since we already add 2* max_pri_tolerance on it.
Let me know if I miss something here.
> That's why I believe we
> need to
> be very cautious when changing it to fix / improve minor issues.
This patch is not for minor fix.
Current DFS detector fails on Japan W53 band which requires at least 50%
of data traffic during DFS certificate.
So this patch should apply to both of 9k and 10k.
> Therefore, if your measurements with 10k reveal that it needs a higher
> tolerance,
> it should be made HW dependent and defined individually for 9k and 10k.
The patch is not to use higher PRI tolerance, but to use more accurate PRI.
>> Signed-off-by: Peter Oh <[email protected]>
>> ---
>> drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c
> b/drivers/net/wireless/ath/dfs_pri_detector.c
>> index 1b5ad19..fef459a 100644
>> --- a/drivers/net/wireless/ath/dfs_pri_detector.c
>> +++ b/drivers/net/wireless/ath/dfs_pri_detector.c
>> @@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {};
>>
>> #define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++)
>> #define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--)
>> +#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \
>> + (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME)
>>
> /* in case of single PRI patterns, take nominal PRI */
> #define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \
> ((MAX - MIN) == 2 * PRI_TOLERANCE) ? MIN + PRI_TOLERANCE :
> RUNTIME)
>
>
> If timestamp jitter is an issue for 10k, you might also want to consider
> adjusting
> the assumed PRI with every pulse you add to a series. Initially I had it
> implemented that way that the PRI was re-calculated continuously to
> minimize the
> variance between nominal and measured pattern. But for the 9k and its
> accurate
> timestamping that turned out to be a lot of calculation for almost no gain
> and was
> dropped.
I also have looked up tsf offset for the issue, but since tsf offset is
applies both of current and previous with similar value,
it didn't help to solve this issue.
>
> Cheers,
> Zefir
>
>
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k
Regards,
Peter
On 03/27/2015 05:59 PM, Peter Oh wrote:
> PRI value is used as divider when DFS detector analyzes candidate
> radar pulses.
> If PRI deviation is big from its origin PRI, DFS detector could miss
> valid radar reports since HW often misses detecting radar pulses and
> causes long interval value of pulses.
>
> For instance from practical results, if runtime PRI is calculated as
> 1431 for fixed PRI value of 1428 and delta timestamp logs 15719,
> the modular remainder will be 1409 and the delta between the remainder
> and runtime PRI is 22 that is bigger than PRI tolerance which is 16.
> As a result this radar report will be ignored even though it's valid.
>
> By using spec defined PRI for fixed PRI, we can correct this error.
>
Hm, not sure if this is a generic solution or 10k specific.
As stated in the source code
/*
* tolerated deviation of radar time stamp in usecs on both sides
* TODO: this might need to be HW-dependent
*/
#define PRI_TOLERANCE 16
the pri tolerance is assumed to be potentially dependent on used HW. The comment
in fact was noted when the detector was written to become generally usable by any
HW out there, before it became Atheros specific.
>From my measurements, the ath9k is far within the given pri tolerance, i.e. if you
continuously feed ETSI pattern 0 over a signal generator, it reports PRIs very
accurately at 1428us (or multiples thereof). Even under heavy load the jitter of
those reported values is within +/- 3us. With that, the ath9k (9580 more
specifically) is already more accurate than the given tolerance - increasing it
further would harm its operability.
The pri tolerance does not only help fine-tuning detection performance, but also
heavily defines whether DFS channels are usable or not (afair, it is the most
significant parameter to reduce false positives). That's why I believe we need to
be very cautious when changing it to fix / improve minor issues.
Therefore, if your measurements with 10k reveal that it needs a higher tolerance,
it should be made HW dependent and defined individually for 9k and 10k.
> Signed-off-by: Peter Oh <[email protected]>
> ---
> drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c b/drivers/net/wireless/ath/dfs_pri_detector.c
> index 1b5ad19..fef459a 100644
> --- a/drivers/net/wireless/ath/dfs_pri_detector.c
> +++ b/drivers/net/wireless/ath/dfs_pri_detector.c
> @@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {};
>
> #define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++)
> #define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--)
> +#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \
> + (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME)
>
/* in case of single PRI patterns, take nominal PRI */
#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \
((MAX - MIN) == 2 * PRI_TOLERANCE) ? MIN + PRI_TOLERANCE : RUNTIME)
If timestamp jitter is an issue for 10k, you might also want to consider adjusting
the assumed PRI with every pulse you add to a series. Initially I had it
implemented that way that the PRI was re-calculated continuously to minimize the
variance between nominal and measured pattern. But for the 9k and its accurate
timestamping that turned out to be a lot of calculation for almost no gain and was
dropped.
Cheers,
Zefir
Peter Oh <[email protected]> writes:
>> As for your patch at hand, I tested it for ETSI and it does not
>> change detector performance,
>
> The patch is useful when there are many missing pulses within a burst.
> It happens almost every time when channel loading rate is higher than
> 40%, but around 30% channel loading does not miss pulses that much.
>
>> therefore (please replace 16 with PRI_TOLERANCE in the macro)
>
> I'll do.
>
>> Acked-by: Zefir Kurtisi <[email protected]>
So what's the conclusion? Should I wait for v2 or is this good to
commit? I didn't quite get Zefir's comment about PRI_TOLERANCE above.
--
Kalle Valo
On 04/15/2015 05:41 AM, Kalle Valo wrote:
> Peter Oh <[email protected]> writes:
>
>>> As for your patch at hand, I tested it for ETSI and it does not
>>> change detector performance,
>> The patch is useful when there are many missing pulses within a burst.
>> It happens almost every time when channel loading rate is higher than
>> 40%, but around 30% channel loading does not miss pulses that much.
>>
>>> therefore (please replace 16 with PRI_TOLERANCE in the macro)
>> I'll do.
>>
>>> Acked-by: Zefir Kurtisi <[email protected]>
> So what's the conclusion? Should I wait for v2 or is this good to
> commit? I didn't quite get Zefir's comment about PRI_TOLERANCE above.
>
Please wait for 2nd patches. I'll prepare the patches that do not void
this current products certificate.
Regards,
Peter
Hi,
On 04/01/2015 03:04 AM, Zefir Kurtisi wrote:
> On 03/30/2015 07:57 PM, Peter Oh wrote:
>> On 03/30/2015 02:55 AM, Zefir Kurtisi wrote:
>>> [...]
>>> That's why I believe we need to
>>> be very cautious when changing it to fix / improve minor issues.
>> This patch is not for minor fix.
>> Current DFS detector fails on Japan W53 band which requires at least 50%
> of data
>> traffic during DFS certificate.
>> So this patch should apply to both of 9k and 10k.
> That is the core of my concern: you add changes to fix FCC/JP, which at
> the same
> time also affects ETSI.
>
> Our company (and maybe others) got ath9k certified for ETSI, and any
> potential
> change to the detector relevant for that domain would essentially require
> to
> re-certify.
>
> There were several patches lately added to the detector that were isolated
> to
> specific domains (like the recent updates for FCC pattern 1) which I knew
> won't
> affect the ETSI detector performance, since they touched only the FCC
> configuration but not the algorithm itself. This patch does, and that's
> why I need
> to point out that doing so might void certification efforts out there.
I'll try to find a way to not affect ETSI detector to keep the existing
certificate.
> Unfortunately, I have no good idea how to cope with it. Freezing the
> driver at the
> certified state is no option, since we all want to evolve. Having multiple
> copies
> of the detector for each regulatory domain would be an option (and
> essentially
> will happen since FCC/JP can't be covered by PRI detectors only), but
> gives
> unacceptable code duplication. Ideally we would fully separate algorithm
> from
> configuration and leave the algorithm untouched ever after, not sure how
> doable,
> though.
>
>
> As for your patch at hand, I tested it for ETSI and it does not change
> detector
> performance,
The patch is useful when there are many missing pulses within a burst.
It happens almost every time when channel loading rate is higher than 40%,
but around 30% channel loading does not miss pulses that much.
> therefore (please replace 16 with PRI_TOLERANCE in the macro)
I'll do.
> Acked-by: Zefir Kurtisi <[email protected]>
>
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k
Thanks,
Peter
On 03/30/2015 07:57 PM, Peter Oh wrote:
>
> On 03/30/2015 02:55 AM, Zefir Kurtisi wrote:
>> [...]
>> That's why I believe we need to
>> be very cautious when changing it to fix / improve minor issues.
> This patch is not for minor fix.
> Current DFS detector fails on Japan W53 band which requires at least 50% of data
> traffic during DFS certificate.
> So this patch should apply to both of 9k and 10k.
That is the core of my concern: you add changes to fix FCC/JP, which at the same
time also affects ETSI.
Our company (and maybe others) got ath9k certified for ETSI, and any potential
change to the detector relevant for that domain would essentially require to
re-certify.
There were several patches lately added to the detector that were isolated to
specific domains (like the recent updates for FCC pattern 1) which I knew won't
affect the ETSI detector performance, since they touched only the FCC
configuration but not the algorithm itself. This patch does, and that's why I need
to point out that doing so might void certification efforts out there.
Unfortunately, I have no good idea how to cope with it. Freezing the driver at the
certified state is no option, since we all want to evolve. Having multiple copies
of the detector for each regulatory domain would be an option (and essentially
will happen since FCC/JP can't be covered by PRI detectors only), but gives
unacceptable code duplication. Ideally we would fully separate algorithm from
configuration and leave the algorithm untouched ever after, not sure how doable,
though.
As for your patch at hand, I tested it for ETSI and it does not change detector
performance, therefore (please replace 16 with PRI_TOLERANCE in the macro)
Acked-by: Zefir Kurtisi <[email protected]>
Zefir Kurtisi <[email protected]> writes:
> On 09/03/2015 08:04 PM, Peter Oh wrote:
>> Hi Zefir,
>>
>> I believe the patch below is good to be checked in since you've already made
>> change in DFS.
>> Can you confirm it?
>>
>> [...]
>>
> Hi Peter,
>
> yes, after adding the chirp detecting feature to ath9k, I did an internal
> certification run with all your patches applied. All good for ETSI (FCC testing
> ongoing), so full ACK for the changes.
I don't have any patch like this in my queue anymore, so please resend
if I need to take something.
--
Kalle Valo
On 09/03/2015 08:04 PM, Peter Oh wrote:
> Hi Zefir,
>
> I believe the patch below is good to be checked in since you've already made
> change in DFS.
> Can you confirm it?
>
> [...]
>
Hi Peter,
yes, after adding the chirp detecting feature to ath9k, I did an internal
certification run with all your patches applied. All good for ETSI (FCC testing
ongoing), so full ACK for the changes.
Cheers,
Zefir
Hi Zefir,
I believe the patch below is good to be checked in since you've already
made change in DFS.
Can you confirm it?
Thanks,
Peter
On 04/01/2015 03:04 AM, Zefir Kurtisi wrote:
> On 03/30/2015 07:57 PM, Peter Oh wrote:
>> On 03/30/2015 02:55 AM, Zefir Kurtisi wrote:
>>> [...]
>>> That's why I believe we need to
>>> be very cautious when changing it to fix / improve minor issues.
>> This patch is not for minor fix.
>> Current DFS detector fails on Japan W53 band which requires at least 50%
> of data
>> traffic during DFS certificate.
>> So this patch should apply to both of 9k and 10k.
> That is the core of my concern: you add changes to fix FCC/JP, which at
> the same
> time also affects ETSI.
>
> Our company (and maybe others) got ath9k certified for ETSI, and any
> potential
> change to the detector relevant for that domain would essentially require
> to
> re-certify.
>
> There were several patches lately added to the detector that were isolated
> to
> specific domains (like the recent updates for FCC pattern 1) which I knew
> won't
> affect the ETSI detector performance, since they touched only the FCC
> configuration but not the algorithm itself. This patch does, and that's
> why I need
> to point out that doing so might void certification efforts out there.
>
> Unfortunately, I have no good idea how to cope with it. Freezing the
> driver at the
> certified state is no option, since we all want to evolve. Having multiple
> copies
> of the detector for each regulatory domain would be an option (and
> essentially
> will happen since FCC/JP can't be covered by PRI detectors only), but
> gives
> unacceptable code duplication. Ideally we would fully separate algorithm
> from
> configuration and leave the algorithm untouched ever after, not sure how
> doable,
> though.
>
>
> As for your patch at hand, I tested it for ETSI and it does not change
> detector
> performance, therefore (please replace 16 with PRI_TOLERANCE in the macro)
>
> Acked-by: Zefir Kurtisi <[email protected]>
>
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k