Is it right to create a parent member function in a child class?

[fl]

Hi,

I am learning GNU Radio code, especially some GIT repository code on Radar.
The code is downloaded from this link:


https://github.com/kit-cel/gr-radar


Of course, it has no guarantee about its correctness. Now I find the C++ code
is puzzling to me. I paste two file contents:
estimator_rcs_impl.cc
estimator_rcs_impl.h

Their parent class contents are not shown here as I don't want it make the
post too long shadowing these two files.

I have two questions.
1. In the beginning of 'estimator_rcs_impl.cc', there is such a parent
member function:



estimator_rcs::sptr
estimator_rcs::make(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent)
{
return gnuradio::get_initial_sptr
(new estimator_rcs_impl(num_mean, center_freq, antenna_gain_tx, antenna_gain_rx, usrp_gain_rx, power_tx, corr_factor, exponent));
}


I don't see it is called by any code in 'estimator_rcs_impl.cc.' This usage
is new to me. Do you think it is normal?


2. The second question is about the comment following the above parent class'
member function. It says:

/*
* The private constructor
*/

in estimator_rcs_impl.cc. But in 'estimator_rcs_impl.h', it says it is a
public function. It looks like a typo for the comment in estimator_rcs_impl.cc.
As I have a light impression that a private constructor is no use generally.
A constructor must be public. Is it correct?

Though the code snippet is about Radar, my question is only about C++.
I am still new to C++. I want to firmly know C++ from it.





Thanks for your reading.









///////////////////////// begin of estimator_rcs_impl.cc
#ifdef HAVE_CONFIG_H
#include "config.h"
#endif

#include <gnuradio/io_signature.h>
#include <boost/circular_buffer.hpp>
#include "estimator_rcs_impl.h"
#include <numeric>

namespace gr {
namespace radar {

estimator_rcs::sptr
estimator_rcs::make(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent)
{
return gnuradio::get_initial_sptr
(new estimator_rcs_impl(num_mean, center_freq, antenna_gain_tx, antenna_gain_rx, usrp_gain_rx, power_tx, corr_factor, exponent));
}

/*
* The private constructor
*/
estimator_rcs_impl::estimator_rcs_impl(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent)
: gr::block("estimator_rcs",
gr::io_signature::make(0,0,0),
gr::io_signature::make(0,0,0))
{
d_num_mean = num_mean;
d_center_freq = center_freq;
d_antenna_gain_tx = antenna_gain_tx;
d_antenna_gain_rx = antenna_gain_rx;
d_usrp_gain_rx = usrp_gain_rx;
d_power_tx = power_tx; // needs to be calibrated for every usage
d_corr_factor = corr_factor;
d_exponent = exponent;

d_rcs_vals.resize(d_num_mean);

// Register input message port
d_port_id_in = pmt::mp("Msg in");
message_port_register_in(d_port_id_in);
set_msg_handler(d_port_id_in, boost::bind(&estimator_rcs_impl::handle_msg, this, _1));

// Register output message port
d_port_id_out = pmt::mp("Msg out");
message_port_register_out(d_port_id_out);

d_loop_counter = 0;

// constant factors in radar equation
d_antenna_gain_abs_rx = pow(10, d_antenna_gain_rx/10);
d_antenna_gain_abs_tx = pow(10, d_antenna_gain_tx/10);
d_lambda = c_light/d_center_freq;
d_fak = pow(4.0*M_PI, 3) / (d_antenna_gain_abs_rx * d_antenna_gain_abs_tx * pow(d_lambda, 2));

}

/*
* Our virtual destructor.
*/
estimator_rcs_impl::~estimator_rcs_impl()
{
}

void
estimator_rcs_impl::set_num_mean(int val){
d_num_mean = val;
d_rcs_vals.clear();
d_rcs_vals.resize(d_num_mean);
d_loop_counter = 0;
}

void
estimator_rcs_impl::set_center_freq(float val){
d_center_freq = val;
d_lambda = c_light/d_center_freq;
d_fak = pow(4.0*M_PI, 3) / (d_antenna_gain_abs_rx * d_antenna_gain_abs_tx * pow(d_lambda, 2));
}

void
estimator_rcs_impl::set_antenna_gain_tx(float val){
d_antenna_gain_tx = val;
d_antenna_gain_abs_tx = pow(10, d_antenna_gain_tx/10);
d_fak = pow(4.0*M_PI, 3) / (d_antenna_gain_abs_rx * d_antenna_gain_abs_tx * pow(d_lambda, 2));
}

void
estimator_rcs_impl::set_antenna_gain_rx(float val){
d_antenna_gain_rx = val;
d_antenna_gain_abs_rx = pow(10, d_antenna_gain_rx/10);
d_fak = pow(4.0*M_PI, 3) / (d_antenna_gain_abs_rx * d_antenna_gain_abs_tx * pow(d_lambda, 2));
}

void
estimator_rcs_impl::set_usrp_gain_rx(float val){
d_usrp_gain_rx = val;
}

void
estimator_rcs_impl::set_power_tx(float val){
d_power_tx = val;
}

void
estimator_rcs_impl::set_corr_factor(float val){
d_corr_factor = val;
}

float
estimator_rcs_impl::calculate_vector_mean(boost::circular_buffer<float>* rcs_vals) {
float sum_of_elems = 0;
for(int k=0; k<rcs_vals->size(); k++){
sum_of_elems += (*rcs_vals)[k];
}
return sum_of_elems/rcs_vals->size();
}

float
estimator_rcs_impl::calculate_rcs() {
// catch errors
if(d_range.size() == 0) throw std::runtime_error("range vector has size zero");
if(d_power.size() == 0) throw std::runtime_error("power vector has size zero");

// regard usrp gain and signal path
float power_rx = pow(d_power[0], d_exponent) / d_power_tx / pow(10, d_usrp_gain_rx/10);

float fak = d_fak * pow(d_range[0], 4);

// debug output
// std::cout << "PowerTx: " << d_power_tx << std::endl;
// std::cout << "PowerRx: " << power_rx << std::endl;
// std::cout << "Lambda: " << d_lambda << std::endl;
// std::cout << "GainRx: " << d_antenna_gain_rx << std::endl;
// std::cout << "GainTx: " << d_antenna_gain_tx << std::endl;
// std::cout << "fak: " << fak << std::endl;

return power_rx/d_power_tx * fak * d_corr_factor;
}

void
estimator_rcs_impl::handle_msg(pmt::pmt_t msg)
{
// Read msg from peak detector
d_msg_hold.clear();
pmt::pmt_t msg_part;
bool found_range = false;
bool found_power = false;
for(int k=0; k<pmt::length(msg); k++){
msg_part = pmt::nth(k,msg);
if(pmt::symbol_to_string(pmt::nth(0,msg_part))=="range"){
d_prange = pmt::nth(1,msg_part);
d_msg_hold.push_back(msg_part);
found_range = true;
}
else if(pmt::symbol_to_string(pmt::nth(0,msg_part))=="power"){
d_ppower = pmt::nth(1,msg_part);
found_power = true;
}
else{
d_msg_hold.push_back(msg_part);
}
}

if(not(found_range&&found_power)) throw std::runtime_error("range or power identifier (symbol) not found");

d_range = pmt::f32vector_elements(d_prange);
d_power = pmt::f32vector_elements(d_ppower);
d_rcs.clear();

if(d_range.size()!=d_power.size()) throw std::runtime_error("range and power vectors do not have same size");

// Calculate RCS
float rcs_mean = 0.0;
if(d_range.size() == 0 && d_power.size() == 0){
//std::cout << "ERROR: No target detected for RCS calculation" << std::endl;
}
else {
d_rcs_vals.push_back(calculate_rcs());
}
//std::cout << "RCS_TEMP: " << calculate_rcs() << std::endl;

if(d_loop_counter+1 >= d_num_mean) {
rcs_mean = calculate_vector_mean(&d_rcs_vals);
}
else {
d_loop_counter++;
}

for(int k=0; k<d_range.size(); k++){
d_rcs.push_back(rcs_mean);
}

// Push pmt to output msg port
d_rcs_key = pmt::string_to_symbol("rcs"); // identifier velocity
d_rcs_value = pmt::init_f32vector(d_rcs.size(), d_rcs); // vector to pmt
d_rcs_pack = pmt::list2(d_rcs_key, d_rcs_value); // make list for velocity information

d_value = pmt::list1(d_rcs_pack);
for(int k=0; k<d_msg_hold.size(); k++){
d_value = pmt::list_add(d_value, d_msg_hold[k]);
}

message_port_pub(d_port_id_out,d_value); // publish message
}

} /* namespace radar */
} /* namespace gr */
///////////////////////// end of estimator_rcs_impl.cc




// begin of estimator_fmcw_impl.h
#ifndef INCLUDED_RADAR_ESTIMATOR_RCS_IMPL_H
#define INCLUDED_RADAR_ESTIMATOR_RCS_IMPL_H

#include <radar/estimator_rcs.h>

namespace gr {
namespace radar {

class estimator_rcs_impl : public estimator_rcs
{
private:
// Nothing to declare in this block

public:
estimator_rcs_impl(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent);
~estimator_rcs_impl();
void handle_msg(pmt::pmt_t msg);
float calculate_rcs();
float calculate_vector_mean(boost::circular_buffer<float>*);
void set_num_mean(int val);
void set_center_freq(float val);
void set_antenna_gain_tx(float val);
void set_antenna_gain_rx(float val);
void set_usrp_gain_rx(float val);
void set_power_tx(float val);
void set_corr_factor(float val);


boost::circular_buffer<float> d_rcs_vals;
int d_num_mean, d_loop_counter;

float d_center_freq, d_antenna_gain_tx, d_antenna_gain_rx, d_usrp_gain_rx, d_power_tx, d_fak, d_lambda, d_antenna_gain_abs_rx,
d_antenna_gain_abs_tx, d_corr_factor, d_exponent;

pmt::pmt_t d_port_id_in, d_port_id_out;
pmt::pmt_t d_prange, d_ppower, d_value;
pmt::pmt_t d_rcs_value, d_rcs_key, d_rcs_pack;
std::vector<float> d_range, d_power, d_rcs;
std::vector<pmt::pmt_t> d_msg_hold;

const static float c_light = 3e8;

};

} // namespace radar
} // namespace gr

#endif /* INCLUDED_RADAR_ESTIMATOR_RCS_IMPL_H */
// end of estimator_fmcw_impl.h

[Bo Persson]

On 2017-10-20 05:04, fl wrote:
> Hi,
>
> I am learning GNU Radio code, especially some GIT repository code on Radar.
> The code is downloaded from this link:
>
>
> https://github.com/kit-cel/gr-radar
>
>
> Of course, it has no guarantee about its correctness. Now I find the C++ code
> is puzzling to me. I paste two file contents:
> estimator_rcs_impl.cc
> estimator_rcs_impl.h
>
> Their parent class contents are not shown here as I don't want it make the
> post too long shadowing these two files.
>
> I have two questions.
> 1. In the beginning of 'estimator_rcs_impl.cc', there is such a parent
> member function:
>
>
>
> estimator_rcs::sptr
> estimator_rcs::make(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent)
> {
> return gnuradio::get_initial_sptr
> (new estimator_rcs_impl(num_mean, center_freq, antenna_gain_tx, antenna_gain_rx, usrp_gain_rx, power_tx, corr_factor, exponent));
> }
>
>
> I don't see it is called by any code in 'estimator_rcs_impl.cc.' This usage
> is new to me. Do you think it is normal?

There are no language rules about which source file should contain an
implementation of a certain function. Having the file name match the
header file makes it a lot easier for ut humans to find the function.

The compiler doesn't really care.

>
>
> 2. The second question is about the comment following the above parent class'
> member function. It says:
>
> /*
> * The private constructor
> */
>
> in estimator_rcs_impl.cc. But in 'estimator_rcs_impl.h', it says it is a
> public function. It looks like a typo for the comment in estimator_rcs_impl.cc.
> As I have a light impression that a private constructor is no use generally.
> A constructor must be public. Is it correct?
>

A constructor doesn't necessarily have to be public. Making it private
restricts who can create objects to friends and member functions of the
class. There could be a static create_object() member, for example.



Bo Persson

[Öö Tiib]

On Friday, 20 October 2017 06:04:24 UTC+3, fl wrote:
> Hi,
>
> I am learning GNU Radio code, especially some GIT repository code on Radar.
> The code is downloaded from this link:
>
>
> https://github.com/kit-cel/gr-radar

In C++ you will see various forms of strange encapsulation. The thing that you
are investigating seems weakly made.

The estimator_rcs enforces usage of boost::shared_ptr despite we have
std::shared_pointer. It does it clumsily and inefficiently and the member
functions of it are pointlessly virtual. Also the comments are
placed sometimes here, sometimes there and hard to follow.

Those weaknesses might not matter in practice with experienced user,
who uses boost anyway and when good documentation is available
elsewhere and when the performance does not matter.

> Of course, it has no guarantee about its correctness. Now I find the C++ code
> is puzzling to me. I paste two file contents:
> estimator_rcs_impl.cc
> estimator_rcs_impl.h

Name containing "impl" usually indicates that it is "private implementation"
class. The users of class estimator_rcs should not include estimator_rcs_impl.h.

>
> Their parent class contents are not shown here as I don't want it make the
> post too long shadowing these two files.

Actually the parent class estimator_rcs is everything what you should deal
with when you want to use it.

>
> I have two questions.
> 1. In the beginning of 'estimator_rcs_impl.cc', there is such a parent
> member function:
>
> estimator_rcs::sptr
> estimator_rcs::make(int num_mean, float center_freq, float antenna_gain_tx, float antenna_gain_rx, float usrp_gain_rx, float power_tx, float corr_factor, float exponent)
> {
> return gnuradio::get_initial_sptr
> (new estimator_rcs_impl(num_mean, center_freq, antenna_gain_tx, antenna_gain_rx, usrp_gain_rx, power_tx, corr_factor, exponent));
> }

That is factory function with what users of estimator_rcs should create
instances of it. It has documenting comment in estimator_rcs.h:

/*!
* \brief Return a shared_ptr to a new instance of radar::estimator_rcs.
*
* To avoid accidental use of raw pointers, radar::estimator_rcs's
* constructor is in a private implementation
* class. radar::estimator_rcs::make is the public interface for
* creating new instances.
*/

> I don't see it is called by any code in 'estimator_rcs_impl.cc.' This usage
> is new to me. Do you think it is normal?

Implementation does not call make, it is for users of it. No it is not normal
but rookieish since he does not use boost::make_shared despite there are
no obstacles to its usage.

>
>
> 2. The second question is about the comment following the above parent class'
> member function. It says:
>
> /*
> * The private constructor
> */
>
> in estimator_rcs_impl.cc. But in 'estimator_rcs_impl.h', it says it is a
> public function. It looks like a typo for the comment in estimator_rcs_impl.cc.
> As I have a light impression that a private constructor is no use generally.
> A constructor must be public. Is it correct?
>
> Though the code snippet is about Radar, my question is only about C++.
> I am still new to C++. I want to firmly know C++ from it.

The "private" here means that the whole estimator_rcs_impl is private
implementation and so its public constructor is private way of making
estimator_rcs.