feedback asked on dab-decoder software in Ada

feedback asked on dab-decoder software in Ada

[jan van katwijk]

Hello group

Last years I did some programming in C++ on SDR-type software. One of the programs is a decoder for DAB(+) signals.
This summer I wanted to learn Ada (again, after a period of well over 20 years) and I  made a reimplementation of the DAB software in Ada.

The resulting Ada program is limited compared to the C++ one in that it is not possible to change dynamically device and DAB mode, and the GUI - I used GtkAda -
is pretty limited.
It does work however fine. Supported input devices are the common dabsticks, the SDRplay device and the airspy.Output is currently using a "default" outputchannel on the PC. 

I would like to get some feedback on the use of the Ada language. I am using bindings to C (some libraries are in C), callbacks from C libraries and quite some tasking.

The sources are available on github: you can download them
by git clone https://github.com/JvanKatwijk/ada-dab.

Any feedback and suggestions for improvement (it definitely runs slower than the C++ version) is welcome

Thanks
jan
 

Re: feedback asked on dab-decoder software in Ada

[Jacob Sparre Andersen]

Jan van Katwijk wrote:

> This summer I wanted to learn Ada (again, after a period of well over
> 20 years) and I made a reimplementation of the DAB software in Ada.

Sounds like an ambitious project.

> I would like to get some feedback on the use of the Ada language.

Some comments and questions:

+ It would make it easier to get contributions from other Ada
  developers, if you switched source style to something closer
  to what is suggested by the Ada Quality and Style Guide [1].

+ Why do you put the package specifications in a separate directory?

+ You might benefit from running your compiler with more warnings
  enabled.

+ A package body doesn't have to "with" itself.

+ There are no guarantees that "Integer" in Ada is the same as "int" in
  C.  If you need a C "int", you should use "Interfaces.C.int".

+ Are you sure you need as many access types as you declare?  (It looks
  - understandably - a bit like you are writing C in Ada.)

+ It looks like you aren't getting as much out of the type system as you
  could.  (A length should probably not be able to contain negative
  values. - Just to take a single example.)

> Any feedback and suggestions for improvement (it definitely runs
> slower than the C++ version) is welcome

I would suggest that you postpone the performance improvements a bit,
and focus on getting more out of Ada.

Greetings,

Jacob

[1] http://www.adaic.org/resources/add_content/docs/95style/html/cover.html
-- 
"For there are only two reasons why war is made against a
 republic: The one, to become lord over her: the other, the
 fear of being occupied by her."       -- Nicolo Machiavelli

Re: feedback asked on dab-decoder software in Ada

[jan van katwijk]

Op donderdag 15 september 2016 11:00:10 UTC+2 schreef Jacob Sparre Andersen:
> Jan van Katwijk wrote:
> 
> > This summer I wanted to learn Ada (again, after a period of well over
> > 20 years) and I made a reimplementation of the DAB software in Ada.
> 
> Sounds like an ambitious project.
> 
> > I would like to get some feedback on the use of the Ada language.
> 
> Some comments and questions:
> 
> + It would make it easier to get contributions from other Ada
>   developers, if you switched source style to something closer
>   to what is suggested by the Ada Quality and Style Guide [1].
> 
> + Why do you put the package specifications in a separate directory?
> 
> + You might benefit from running your compiler with more warnings
>   enabled.
> 
> + A package body doesn't have to "with" itself.
> 
> + There are no guarantees that "Integer" in Ada is the same as "int" in
>   C.  If you need a C "int", you should use "Interfaces.C.int".
> 
> + Are you sure you need as many access types as you declare?  (It looks
>   - understandably - a bit like you are writing C in Ada.)
> 
> + It looks like you aren't getting as much out of the type system as you
>   could.  (A length should probably not be able to contain negative
>   values. - Just to take a single example.)
> 
> > Any feedback and suggestions for improvement (it definitely runs
> > slower than the C++ version) is welcome
> 
> I would suggest that you postpone the performance improvements a bit,
> and focus on getting more out of Ada.
> 
> Greetings,
> 
> Jacob
> 
> [1] http://www.adaic.org/resources/add_content/docs/95style/html/cover.html
> -- 
> "For there are only two reasons why war is made against a
>  republic: The one, to become lord over her: the other, the
>  fear of being occupied by her."       -- Nicolo Machiavelli

Thanks for the comments. In the meantime I tried to apply some guideline rules, and the gnat compiler does not complain anymore on the layout.
Wrt identifiers: I am not impressed very much by the guidelines, to me Is_Running is not better readable than isRunning, Has_Property not than hasProperty.

First of all: the ada implementation of DAB is derived from the existing C++ implementation, all algorithms were extensively tried and tested in C++, so the complexity was more on using C libraries (I use several C libraries), some of them with callbacks. That all went pretty smooth, however, handling the GUI turned out to be more difficult than hndling the much more complex GUI in C++ (GtkAda vs Qt) 

The code and coding style is obviously influenced by having programmed in C++ for the last 10 years. Wrt access types, I really have to look at it, most likely it follows from the C++ background.

Anyway, thanks for the advices, I'll continue to make it more "Ada-style"

jan

Re: feedback asked on dab-decoder software in Ada

[jan van katwijk]

Op zaterdag 24 september 2016 19:45:26 UTC+2 schreef jan van katwijk:
> Op donderdag 15 september 2016 11:00:10 UTC+2 schreef Jacob Sparre Andersen:
> > Jan van Katwijk wrote:
> > 
> > > This summer I wanted to learn Ada (again, after a period of well over
> > > 20 years) and I made a reimplementation of the DAB software in Ada.
> > 
> > Sounds like an ambitious project.
> > 
> > > I would like to get some feedback on the use of the Ada language.
> > 
> > Some comments and questions:
> > 
> > + It would make it easier to get contributions from other Ada
> >   developers, if you switched source style to something closer
> >   to what is suggested by the Ada Quality and Style Guide [1].
> > 
> > + Why do you put the package specifications in a separate directory?
> > 
> > + You might benefit from running your compiler with more warnings
> >   enabled.
> > 
> > + A package body doesn't have to "with" itself.
> > 
> > + There are no guarantees that "Integer" in Ada is the same as "int" in
> >   C.  If you need a C "int", you should use "Interfaces.C.int".
> > 
> > + Are you sure you need as many access types as you declare?  (It looks
> >   - understandably - a bit like you are writing C in Ada.)
> > 
> > + It looks like you aren't getting as much out of the type system as you
> >   could.  (A length should probably not be able to contain negative
> >   values. - Just to take a single example.)
> > 
> > > Any feedback and suggestions for improvement (it definitely runs
> > > slower than the C++ version) is welcome
> > 
> > I would suggest that you postpone the performance improvements a bit,
> > and focus on getting more out of Ada.
> > 
> > Greetings,
> > 
> > Jacob
> > 
> > [1] http://www.adaic.org/resources/add_content/docs/95style/html/cover.html
> > -- 
> > "For there are only two reasons why war is made against a
> >  republic: The one, to become lord over her: the other, the
> >  fear of being occupied by her."       -- Nicolo Machiavelli
> 
> Thanks for the comments. In the meantime I tried to apply some guideline rules, and the gnat compiler does not complain anymore on the layout.
> Wrt identifiers: I am not impressed very much by the guidelines, to me Is_Running is not better readable than isRunning, Has_Property not than hasProperty.
> 
> First of all: the ada implementation of DAB is derived from the existing C++ implementation, all algorithms were extensively tried and tested in C++, so the complexity was more on using C libraries (I use several C libraries), some of them with callbacks. That all went pretty smooth, however, handling the GUI turned out to be more difficult than hndling the much more complex GUI in C++ (GtkAda vs Qt) 
> 
> The code and coding style is obviously influenced by having programmed in C++ for the last 10 years. Wrt access types, I really have to look at it, most likely it follows from the C++ background.
> 
> Anyway, thanks for the advices, I'll continue to make it more "Ada-style"
> 
> jan

I looked into the abundant use of access types. In the front end (i.e. the ofdm processing part) some access varaibles could be eliminated and replaced by "normal" variables. In the backend, however, quite some dynamic choices determine which part of the program should be active (e.g. MP2 decoding or MP4 decoding).

jan

Re: feedback asked on dab-decoder software in Ada

[G.B.]

On 24.09.16 20:38, jan van katwijk wrote:
> I looked into the abundant use of access types. In the front end (i.e. the ofdm processing part) some access varaibles could be eliminated and replaced by "normal" variables. In the backend, however, quite some dynamic choices determine which part of the program should be active (e.g. MP2 decoding or MP4 decoding).

I'm guessing only, not having looked closely, but just in case:
the way to specify dynamic choice of types etc. is by declaring
variables and parameters to be of type T'Class, similar to C++'s T&:

struct S {};
bool operator==(const S& left, const S& right) { return true; };

/// calling op `foo` for parameters of any type derived from T.
S f(const T& thing) {
    return thing->foo(1);
}

class T {
public:
   T() {}
   virtual S foo(int) const = 0;
};

/// T1, T2 types derived from T

int main() {
   T1 x = T1();
   T2 y = T2(4);
   return (f(x) == f(y));
}


package N is

    type S is record
       null;
    end record;

    type T is abstract tagged null record;

    function foo(thing: T; X : Natural) return S is abstract;

    -- T1, T2 types derived from T

end N;

with N;
-- calling op `foo` for parameters of any type derived from T.
function F(Thing: N.T'Class) return N.S is
begin
    return Thing.Foo(1);
end F;

with N, F;  use N;
function Main return Integer is
    X : T1;
    Y : T2 := Construct (4);
begin
    return Boolean'Pos (F(X) = F(Y));
end Main;


-- 
"HOTDOGS ARE NOT BOOKMARKS"
Springfield Elementary teaching staff

Re: feedback asked on dab-decoder software in Ada

[Simon Wright]

jan van katwijk <j.vankatwijk@gmail.com> writes:

> Op donderdag 15 september 2016 11:00:10 UTC+2 schreef Jacob Sparre Andersen:
>> Jan van Katwijk wrote:

>> + It would make it easier to get contributions from other Ada
>>   developers, if you switched source style to something closer
>>   to what is suggested by the Ada Quality and Style Guide [1].

> Wrt identifiers: I am not impressed very much by the guidelines, to me
> Is_Running is not better readable than isRunning, Has_Property not
> than hasProperty.

But Jacob is writing from the viewpoint of other Ada developers. I would
personally find using CamelCase very awkward; my IDE takes care of most
Ada-style casing for me, I would have to put isRunning into a special
exception (and the leading 'l' would pretty much mandate that).

>> + Why do you put the package specifications in a separate directory?

I've noticed this a lot recently looking at Ada translations from C
originals. In some cases (e.g. this commit[1]) developers have reverted
to a more 'normal' Ada style, specs & bodies in the same directory and
no '_Pack' on the end of package names!


All this is of course personal preference.

[1] https://github.com/AdaCore/Certyflie/commit/c98fd04ce811b3009de7dd7ab39b0359041b42ff

Re: feedback asked on dab-decoder software in Ada

[jan van katwijk]

Yes, but the choices here are made on user input. The first step is that the dab software looks for a set of programs belonging to an ensemble. Of course, the user may select a different channel, such that the software has to start all over again and look for the data in the ensemble, encoded in the channel.
Then, the user may select a program from a list. That program has attributes needed for decoding (e.g. protection level influencing the deconvolution, MP2 or MP4 etc)

For such - pretty major - changes I selected - obviously influenced by having done the implementation in C++ earlier - to delete whole instances and create new one with the new parameters, and yes, I am using class wide variables to refer to these new instances (e.g. the MP2 handler and the MP4 handler both derive from a single - empty - audio class. 

Wrt style: I like writing things in neat colums, use lots of white space etc etc. But I am too old to get used to an IDE (I prefer working from the command line, using VI), and for me it is equal effort to Capitalize or not. But again, for things like "is..." "has..." I definitely prefer lowercases in front, rather than "Is_XXX" or "Has_XXX". Well, as said, Gnat does not complain anymore about the layout and I am looking at naming.

One of the nicer things in C(++) is the larger degree in freedom in introducing a new variable. I really prefer localizing, but in Ada that leads to pretty heavy nesting if blocks in other structures (i.e. assume you need a variable in one branch of an if-then-else or within a loop)

Anyway, it is a real fun project
jan