Re: [Ptpd-devel] Coding conventions, code complexity, refactoring and other vulgar words that we should not talk before midnight

[Jan B. <jan...@ja...>]

2013/5/30 George Neville-Neil <gn...@ne...>

>
> On May 30, 2013, at 14:32 , Jan Breuer <jan...@ja...> wrote:
>
> > Hi all,
> > I have a problem with the code of PTPd. It was almost beautiful peace of
> code before few years, but now it is bloated and illegible.
> >
>
> I beg to disagree.  The PTPd code has always only been partially legible.
>  But, let's talk more about this below.
>

In times of PTP 1, it was much better, but you are obviously right.


>
> > I use this code in our project. We are working on small embedded devices
> synchronized by PTP. They are running FreeRTOS and LwIP stack with 128kB
> RAM.
> >
> > Everything works great, but it is harder and harder to update to new
> versions of PTPd.
> >
> > These changes are not popular and can add some new bugs. I know it. I
> can do lot of work as a volunteer.
>
> Great!  We love volunteers.
>
> > Chapter 1: Design of PTPd
> > ====================
> > PTPd is divided into many files. In the src directory, there should be
> platform independent code and in the dep directory, there should be
> platform and network dependent code. There is nice file display.c, but
> there are other fuction displaying somethin everywhere in the code. eg.
> display..., translate..., functions in sys.c
> >
> > sys.c should by smallest possible abstraction layer between time
> functions of OS and PTPd.
> >
>
> Sure.  I think you're not the only one to think this.
>

OK, I can start with this, because it should not break anything, just move
functions to right files.


>
> >
> > Chapter 2: Pseudo object programming
> > ============================
> > There are some conventions writing object oriented code in pure C.
> Object structure should be the first or the last parameter of all "class
> functions".
> >
> > correct example:
> > netSend(NetPath*, buf, length, other parameters)
> >
> > wrong example from the code:
> > netSendEvent(Octet * buf, UInteger16 length, NetPath * netPath,
> RunTimeOpts *rtOpts, Integer32 alt_dst)
> >
> > "Class structure" NetPath is the middle parameter of netSendEvent.
> >
>
> That's a convention, but, yes, it's not a bad idea to swap that around.
>
> >
> > Chapter 3: Naming conventions
> > ======================
> > There are many naming conventions and all of them are used in PTPd
> source code.
> >
> > someName
> > SomeName
> > some_name
> > SOME_NAME
> >
> > I thing, that best practise for PTPd project is:
> > - all constants are written as "SOME_NAME"
> > - all variables and functions are written as "someName"
> > - all custom types are written as "SomeName"
> >
> > Writing function, type or variable with name "some_name" should be
> prohibited.
> >
> >
>
> If I had the time to rewrite the whole daemon I would put it into style(9)
> and get rid
> of all the bouncy caps.
>

syle(9) is fine. There are two things:
1) I personnaly prefere naming corresponding with the spec and they are
using camelCase. Not prefered but allowed in FreeBSD style(9)
2) I personnaly prefere creating function prototypes with parameter names.
void    function(int _fd, int  _delay, int _offset);
and not
void    function(int, int, int);
which is source of errors for me.



>
> > Chapter 4: Code complexity
> > ======================
> > Functions longer than one screen are hardly readable. Using copy&paste
> instead of writing function is not good idea.
> >
> > Bad example of copy&paste are netRecvEvent and netRecvGeneral,
> netSend*,..
> > Bad example of code complexity are netRecv*, netInit,
> bmcDataSetComparison, ptpdStartup and many others.
> >
> > Many of these functions can be divided to smaller peaces of code. They
> can share some code, etc.
> >
>
> Actually the right answer here, and one I wish to pursue, is to break up
> all the net* functions so
> that we can have a more object like lower layer.  Now that we have
> multiple transports that's
> more necessary than it was before.  The code as it stands definitely is
> not the way I'd ultimately
> like it to be.
>
>
I thing, it is time to create one file for every transport. We can add some
new fields to NetPath structure - recvEvent, recvGeneral, sendEvent, ...
and initialize this by correct functions. I can prepare some draft of this.




> > Chapter 5: Memory consuptions
> > ========================
> > As an embedded systems developer, it hurts me every wasted byte of RAM.
> There are many function allocating unnecessary memory eg.
> dump_TimeInternal, dump_TimeInternal2, time2st, ether_ntohost_cache and
> others. Every function allocates 1kB array and they use maybe 30 bytes.
> >
>
> That's a concern, certainly, on embedded systems.  If you want to clean
> that up I'd love to see
> the patch.  I spend a lot of time on embedded platforms and also abhor the
> waste.
>
> > Chapter 6: Future of the PTPd
> > ========================
> > What are you planning for the PTPd in the future. There could be many
> new features, but now it is hard to add them.
> > - IPv6 (does anybody use it?)
> > - real Boundary Clock
> > - tickless timers (I already use them on my boards)
> > - safe interface for other systems - e.g. GPS with direct setting of
> clockClass, UTC offset, etc. - using PTPd as a library or as a stand alone
> daemon.
> >
>
> Don't forget that there's also a working group (some of us on this list
> are involved) looking at
> the next version of IEEE-1588, that's going to require use to extend the
> code further.


Of course, I forget to write that but I know it. Are they planning to make
new version backward compatible and add few more target specific profiles
(like White Rabbit)?


> >
> > Tell me what you thing about it. If you want to leave PTPd as is -
> bloated and ossified - or change something. I will be happy to do all these
> changes and you can just review them.
>
> I think the list above is a good one but we need a few other things first.
>  One of them, which is coming soon,
> is configuration files, as our current system of using every letter of the
> alphabet is intractable.
>
> The other feature I care about will be talking to the kernel about
> different types of timekeeping.
> With BPF/PCAP on FreeBSD you'll be able to select the quality of the time
> you get from
> the kernel.  That's yet another option.  After that we need a clean API
> for telling the kernel to use
> hardware timestamping.
>

I don't forget these, but it is already changing, so it is present for me.


>
> In terms of code clarity, getting the code to confrom completely to
> style(9) would be a good
> step towards readability.  One thing that annoys me quite a bit is the
> incredible indenting, long names,
> and then the mix with functions like s1.  It's all well and good to name a
> function after the state
> in the spec, but, really, s1, is not a good function name.
>
> If this is all something that interests you I suggest you start out with
> smaller patches, as they're easier
> for us all to evaluate, and that will allow us to integrate the fixes more
> easily.  A jumbo white space cleanup
> will just make doing the 2.3 release harder.
>
>
>
I don't want to creat jumbo patch and I don't want to break it at all. I
just want to know, if you are open for some changes. I can create some
patches and I will send them for review.

Regards,
Jan