All of lore.kernel.org
 help / color / mirror / Atom feed
From: Felix Fietkau <nbd@openwrt.org>
To: ath9k-devel@lists.ath9k.org
Subject: [ath9k-devel] [RFC 1/2] ath9k: add DFS pattern detector
Date: Fri, 27 Jan 2012 16:22:10 +0100	[thread overview]
Message-ID: <4F22C122.5070704@openwrt.org> (raw)
In-Reply-To: <4F22BE44.4000705@neratec.com>

On 2012-01-27 4:09 PM, Zefir Kurtisi wrote:
> 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.
Yes, I'm talking about the risk of those two diverging. It may be a
small risk right now, but people tend to forget about things like that
over time, and then it becomes bigger. Also, this is very easy to avoid
in a way that does not change the compiled binary code in any way.

- Felix

WARNING: multiple messages have this Message-ID (diff)
From: Felix Fietkau <nbd@openwrt.org>
To: Zefir Kurtisi <zefir.kurtisi@neratec.com>
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:22:10 +0100	[thread overview]
Message-ID: <4F22C122.5070704@openwrt.org> (raw)
In-Reply-To: <4F22BE44.4000705@neratec.com>

On 2012-01-27 4:09 PM, Zefir Kurtisi wrote:
> 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.
Yes, I'm talking about the risk of those two diverging. It may be a
small risk right now, but people tend to forget about things like that
over time, and then it becomes bigger. Also, this is very easy to avoid
in a way that does not change the compiled binary code in any way.

- Felix

  reply	other threads:[~2012-01-27 15:22 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     ` [ath9k-devel] " Zefir Kurtisi
2012-01-27 15:09       ` Zefir Kurtisi
2012-01-27 15:22       ` Felix Fietkau [this message]
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=4F22C122.5070704@openwrt.org \
    --to=nbd@openwrt.org \
    --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.