by Marcus Wolf

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

Am 06.12.2017 um 21:57 schrieb Simon Sandström:
> On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>>>
>>>
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>>>
>>>>
>>>> Wow... This was the one patch I thought was going to sink this
>>>> patchset...
>>>
>>> I don't get that. What do you mean?
>>>
>>>> Isn't enum optionOnOff part of the userspace headers?
>>>>
>>>> I thought we weren't allowed to change that.
>>>
>>> All enums are for user space (or inteded to be used in userspace in future).
>>> Didn't introduce enums for internal use.
>>
>> So what I'm asking is if we do this change, does it break any userspace
>> programs which are used *right now*. In other words will programs stop
>> compiling because we've renamed an enum?
>>
>> regards,
>> dan carpenter
>>
>
> Hi,
>
> I'm not sure about this so I have to ask: wouldn't the header need to be
> in include/uapi/ for userspace to use them? Or is it "allowed" for
> userspace programs to use this internal header? Or are we afraid to
> break userspace programs that keeps a copy of pi433_if.h?
>
>
> Regards,
> Simon
>

Hi Simon,

in principle, I think you are right. With my first patch, offering the
driver I did it that way, but Greg asked me to migrate everything
(including docu) into staging/pi433. I think, that's related to being in
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace
program, in order to be able to compile it.

To be honest, I am a little bit astonished about that question. Don't
you do a regression test after such a redesign? I would be a little bit
afraid to offer such redesign without testing.

Regards,
Marcus

2017-12-07 09:41:46

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

On Thu, Dec 07, 2017 at 12:17:32AM +0200, Marcus Wolf wrote:
> To be honest, I am a little bit astonished about that question. Don't you do
> a regression test after such a redesign? I would be a little bit afraid to
> offer such redesign without testing.

He probably doesn't have the hardware... That's fine. Most patches to
staging are like this. We have a pretty good track record for rarely
introducing bugs.

This patchset is pretty easy to review with confidence because it's
mostly just renames so there are only a few bits to review by hand.
I've attached the perl script I use for reviewing renames and indent
changes.

regards,
dan carpenter

Attachments:

(No filename) (656.00 B)
rename_rev.pl (11.65 kB)
Download all attachments

2017-12-07 09:47:19

by Marcus Wolf

[permalink] [raw]

Subject: staging: pi433: Plans from Smarthome-Wolf

Am 06.12.2017 um 14:47 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:11:27AM +0200, Marcus Wolf wrote:
>>
>> Since the rule for kernel development seems to be, not to care about future,
>> most probably you patch is fine, anyway.
>>
>
> Yeah. Deleting code if there is no user is required to move this code
> out of staging...
>
> I've worked in staging for a long time and I've seen patches from
> thousands of people. Simon really seems to understand kernel style and
> that's less common than one might think. It's a valuable skill. If you
> would just leave him to work then this driver would get fixed...
>
> You're making it very difficult because you're complaining about the
> work which *needs* to be done. It's discouraging for people sending
> patches. This is very frustrating... :(
>
> On the naming, I think having a function which is named "enable" but
> actually disables a feature is a non-starter. What about we do either
> one of these:
> pi433_enable_<feature>(spi);
> pi433_disable_<feature>(spi);
> Or:
> pi433_set_<feature>(spi, SETTING);
>
> It's still a standard and we'll just decide on a case by case basis what
> to use. This is a tiny driver and it's really easy to grep for the
> feature name to find the functions you want. Plus all the config is
> done in one location so it's really not that hard.
>
> regards,
> dan carpenter
>

Hi Greg, Dan, Simon and all others,

yesterday we had a project meeting. Though I am the boss, I was
"punished" that I spend such a bunch of time to discuss here, instead of
finishing stuff from my todo list :-P

I get the point, that I am not really deep in the kernel style guide and
it is better, if I don't disturb the nerds.

Both facts in combination tell me, that I should lean back, wait a while
and see, what the driver will become.

On the other hand, my team was - like me - a little bit in worry about
this "closing a door", that happend a few times during the last weeks
and the possible redesign of the driver architecture. It would be pitty,
if the effort for integration of new features will be complicated a lot.

We finally decided, that I actually should reduce focussing on the
driver for the moment and let things go. We'd like to use this mail, to
tell our ideas and the plan for next year:

When submitting the driver, we wanted to add support to the kernel for
the technical really good modules from HopeRf. The idea was to support
serveral chips and several modules (carrying those chips). Due to
financial and time restrictions, we finally had to reduce to rfm69cw and
focused on Pi433.
Plan for the next year:
* Tweak the driver (if needed) to enable the reception of telegrams
without preamble and sync pattern. We never tested this before. This
feature will be needed aprox. in March'18.
* Support for the rfm69cw-868 - Almost the same module as the current,
but for the 868MHz ISM band. Will be needed approx. end of summer next year.
* We would like to add support for some more features of the chip (e. g.
AES) - you can see, there are lots of so far not covered registers
(commented lines in rf69_registers.h). No shedule for this.
* If we will have a (financially) successfull 2018, we think about
integration of the rf95 chip.

>From third parties we were asked about the follwoing features:
* Implement support for continuous mode (e. g. from projects fhem,
domotics).
* Do a little bit more generic inteface implementation, to also support
other hardware setups, using the rf69 chip.
Second sounds very interesting - especially in terms of a real Linux
driver. But both topics so far aren't on our agenda.

So I will withdraw from the development of pi433 driver for the next
monthes and will be back at the beginning of spring. I hope, there are
not too many closed doors, so I can easily start over with head rev. and
don't have to fall back to my old base. To ease a start over, I would
love the following things (as much, as possible without breaking the rules):
* do not modify the register abstraction in a way, that it doesn't
represent the real hardware any more (e. g. the stuff with amp - the
chip does not have two registers, taking chipnumbers, but 3 bits, taking
on/off information). In doubt have a look at the data sheet.
* stay with the naming convention for the register abstraction
(rf69_set_... and rf69_get_...). If for some reason a register (or bit)
needs to be read back for some reason in future, it is unlovely to have
rf69_do_something and you need to find a adequate name for the getter.
rf69_get_do_something is ugly most times.
* if possible, do not remove my tiny "debug system" in the register
abstraction. It's very powerfull, if you work on config changes.
* try to keep the current or find a new way, that a setting from user
space could be mapped to (not identical) register sets of different chips.

If you have any questions - especially concerning the hardware facts -
feel free to ask. To be sure, I don't miss the mail between others from
the mailing list, write something like Hi Marcus! in the first line or
even in the subject.

Thanks a lot for your effort!

Merry Christmas and a happy new year,

Marcus