All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
To: ath9k-devel@lists.ath9k.org
Subject: [ath9k-devel] [RFC 1/2] ath9k: add DFS pattern detector
Date: Fri, 27 Jan 2012 16:09:56 +0100	[thread overview]
Message-ID: <4F22BE44.4000705@neratec.com> (raw)
In-Reply-To: <4F2176DE.9010604@openwrt.org>

On 01/26/2012 04:53 PM, Felix Fietkau wrote:
> On 2012-01-26 4:34 PM, Zefir Kurtisi wrote:
> [...]
>> +/**
>> + * struct pattern_detector - overloading base dfs_pattern_detector
>> + *
>> + * @exit(): destructor
>> + * @add_pulse(): add radar pulse to detector
>> + * @num_radar_types: number of different radar types
>> + * @last_pulse_ts: time stamp of last valid pulse
>> + * @radar_detector_specs: array of radar detection specs
>> + * @channel_detectors: list connecting channel_detector elements
>> + */
>> +struct pattern_detector {
>> +	void (*exit)(struct pattern_detector *_this);
>> +	enum dfs_detector_result (*add_pulse)
>> +		(struct pattern_detector *_this, struct pulse_event *pe);
>> +
>> +	u8 num_radar_types;
>> +	u64 last_pulse_ts;
>> +	struct radar_detector_specs *radar_spec;
>> +	struct list_head channel_detectors;
>> +};
> To overload it this way is quite fragile. It's better to embed struct
> dfs_pattern_detector here. In places where you need to go from the
> struct dfs_pattern_detector to this struct, you can then use the
> container_of macro, to get at least some form of type safety.
> 
> - Felix

Hi Felix,

thanks for taking a look.

Actually, for this initial post we do not need any polymorphism at all, I could basically make this derived class the interface in the PATCH to come.

Though (since I am used to this coding style) I'd like to understand your concern. By 'fragile' you are referring to the risk of base and derived class diverging, right? If it is that, I see that one is in trouble if you let that happen. Here with the base class being defined as external interface holding just two function pointers I thought the risk is negligible.

But generally you're right of course. I'll take it into account for the PATCH.


Thanks
Zefir

WARNING: multiple messages have this Message-ID (diff)
From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
To: Felix Fietkau <nbd@openwrt.org>
Cc: ath9k-devel@lists.ath9k.org, linux-wireless@vger.kernel.org,
	rodrigue@qca.qualcomm.com, adrian.chadd@gmail.com,
	kgiori@qca.qualcomm.com, shafi.wireless@gmail.com
Subject: Re: [RFC 1/2] ath9k: add DFS pattern detector
Date: Fri, 27 Jan 2012 16:09:56 +0100	[thread overview]
Message-ID: <4F22BE44.4000705@neratec.com> (raw)
In-Reply-To: <4F2176DE.9010604@openwrt.org>

On 01/26/2012 04:53 PM, Felix Fietkau wrote:
> On 2012-01-26 4:34 PM, Zefir Kurtisi wrote:
> [...]
>> +/**
>> + * struct pattern_detector - overloading base dfs_pattern_detector
>> + *
>> + * @exit(): destructor
>> + * @add_pulse(): add radar pulse to detector
>> + * @num_radar_types: number of different radar types
>> + * @last_pulse_ts: time stamp of last valid pulse
>> + * @radar_detector_specs: array of radar detection specs
>> + * @channel_detectors: list connecting channel_detector elements
>> + */
>> +struct pattern_detector {
>> +	void (*exit)(struct pattern_detector *_this);
>> +	enum dfs_detector_result (*add_pulse)
>> +		(struct pattern_detector *_this, struct pulse_event *pe);
>> +
>> +	u8 num_radar_types;
>> +	u64 last_pulse_ts;
>> +	struct radar_detector_specs *radar_spec;
>> +	struct list_head channel_detectors;
>> +};
> To overload it this way is quite fragile. It's better to embed struct
> dfs_pattern_detector here. In places where you need to go from the
> struct dfs_pattern_detector to this struct, you can then use the
> container_of macro, to get at least some form of type safety.
> 
> - Felix

Hi Felix,

thanks for taking a look.

Actually, for this initial post we do not need any polymorphism at all, I could basically make this derived class the interface in the PATCH to come.

Though (since I am used to this coding style) I'd like to understand your concern. By 'fragile' you are referring to the risk of base and derived class diverging, right? If it is that, I see that one is in trouble if you let that happen. Here with the base class being defined as external interface holding just two function pointers I thought the risk is negligible.

But generally you're right of course. I'll take it into account for the PATCH.


Thanks
Zefir

  reply	other threads:[~2012-01-27 15:09 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-26 15:34 [ath9k-devel] [RFC 0/2] ath9k: DFS pattern detector Zefir Kurtisi
2012-01-26 15:34 ` Zefir Kurtisi
2012-01-26 15:34 ` [ath9k-devel] [RFC 1/2] ath9k: add " Zefir Kurtisi
2012-01-26 15:34   ` Zefir Kurtisi
2012-01-26 15:53   ` [ath9k-devel] " Felix Fietkau
2012-01-26 15:53     ` Felix Fietkau
2012-01-27 15:09     ` Zefir Kurtisi [this message]
2012-01-27 15:09       ` Zefir Kurtisi
2012-01-27 15:22       ` [ath9k-devel] " Felix Fietkau
2012-01-27 15:22         ` Felix Fietkau
2012-01-26 19:25   ` [ath9k-devel] " David Goodenough
2012-01-27  0:53     ` Adrian Chadd
2012-01-26 15:34 ` [ath9k-devel] [RFC 2/2] ath9k/dfs_pattern_detector: add standalone testing Zefir Kurtisi
2012-01-26 15:34   ` Zefir Kurtisi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F22BE44.4000705@neratec.com \
    --to=zefir.kurtisi@neratec.com \
    --cc=ath9k-devel@lists.ath9k.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.