PDA

View Full Version : Am I going to Qt Hell for doing it this way? (reading binary data into a program)



m_bishop
15th May 2013, 17:31
Problem statement: Is there a better way? What is the Qt way?

My objective is to read data from a binary file into my object and then do work on it. The binary file is made up of rows that are 16 bytes long. The data in each row has unique meaning and format and I only care about one row. The format of the row at CONFIG_USOB_ADDR is defined by the struct{}, which just so happens to have a union in it that describes some bit fields. And this all needs to happen when I click on a QPushButton.

Now, this works below but I don't know if there would be a better way in Qt. What would my options be?

Class Header


namespace Ui {
class new_IPC_Dialog;
}

union UnitType
{
unsigned short UnitAddress;
struct
{
unsigned short N_Unit : 6;
unsigned short Subzone : 4;
unsigned short Zone : 6;
};
};

// common structure for FromAddress and ToAddress.
typedef struct tagFrameAddress
{
unsigned short Group;
unsigned short Site;
union UnitType Unit;
unsigned short SubUnit;
unsigned short Object;
} FrameAddress;

class new_IPC_Dialog : public QDialog
{
Q_OBJECT

public:
explicit new_IPC_Dialog(QWidget *parent = 0);
~new_IPC_Dialog();

void update_config(const QString &zsu_address);
void start_ipc(const QString mac_address);

public slots:
void on_IPC_start_clicked();

private:
Ui::new_IPC_Dialog *ui;
FrameAddress my_USOB;
};


Class Implementation:


#define CONFIG_USOB_ADDR 0x0010
#define CONFIG_USOB_LENGTH 0x0A

#define CONFIG_USOB_FIELD_LENGTH 0x02
#define CONFIG_USOB_GROUP_OFFSET 0x00
#define CONFIG_USOB_SITE_OFFSET 0x02
#define CONFIG_USOB_UNIT_OFFSET 0x04
#define CONFIG_USOB_SUBUNIT_OFFSET 0x06
#define CONFIG_USOB_OBJECT_OFFSET 0x08

new_IPC_Dialog::new_IPC_Dialog(QWidget *parent) :
QDialog(parent),
ui(new Ui::new_IPC_Dialog)
{
ui->setupUi(this);
}

new_IPC_Dialog::~new_IPC_Dialog()
{
delete ui;
}

void new_IPC_Dialog::update_config(const QString &zsu_address)
{

QString ipc_config_location = "/usr/local/ipc_simulator/";
QString ipc_default_config_name = "default_config.bin";
QString ipc_config_name = "default_config"+zsu_address+".bin";

QFile file(ipc_config_location+ipc_config_name);

if(!file.open(QIODevice::ReadOnly))
return;

file.seek(CONFIG_USOB_ADDR);

QByteArray buffer = file.read(CONFIG_USOB_LENGTH);

bzero(&this->my_USOB, sizeof(FrameAddress));
memcpy(&(this->my_USOB.Group), buffer.data() + CONFIG_USOB_GROUP_OFFSET, CONFIG_USOB_FIELD_LENGTH);
memcpy(&(this->my_USOB.Site), buffer.data() +CONFIG_USOB_SITE_OFFSET, CONFIG_USOB_FIELD_LENGTH);
memcpy(&(this->my_USOB.Unit), buffer.data() + CONFIG_USOB_UNIT_OFFSET, CONFIG_USOB_FIELD_LENGTH);
memcpy(&(this->my_USOB.SubUnit), buffer.data() + CONFIG_USOB_SUBUNIT_OFFSET, CONFIG_USOB_FIELD_LENGTH);
memcpy(&(this->my_USOB.Object), buffer.data() + CONFIG_USOB_OBJECT_OFFSET, CONFIG_USOB_FIELD_LENGTH);


//Do some really cool stuff based on the values in the my_USOB thingy.

file.close();
}

void new_IPC_Dialog::start_ipc(const QString mac_address)
{
}

void new_IPC_Dialog::on_IPC_start_clicked()
{
QString mac_address = ui->mac_address->text();
QString zsu_address = ui->zsu_address->text();

update_config(zsu_address);
start_ipc(mac_address);

ui->mac_address->clear();
ui->zsu_address->clear();
hide();
}

Santosh Reddy
15th May 2013, 17:51
It looks just fine, only thing I like to suggest is that if possible try to separate the file reading logic from QDialog (Ui), may be create a different class (or even global functon) as FrameReader, then use the FrameReader instance from QDialog to read the data into my_USOB. This way QDialog will not be depending on the way you read the file (or file structure or format). This will gives you an option of replacing the FrameReader in future (if required).

m_bishop
15th May 2013, 18:02
Seriously? I'm glad that didn't make your eyes bleed. I'm just starting out with Qt and I'm starting to like it. But, alas, I'm a C guy and the last C++ I did was in the 90s. So, I'm having to relearn C++ at the same time as Qt.

anda_skoa
15th May 2013, 23:37
Getting rid of the "this->" would improve readability a bit, but I agree that it is mostly fine.

Cheers,
_

m_bishop
16th May 2013, 15:51
Getting rid of the "this->" would improve readability a bit, but I agree that it is mostly fine.

Cheers,
_

What would be a generally good practice on doing that?

amleto
17th May 2013, 01:10
just dont do it.

also, memcpy is very unsafe and I can't see why you'd bother for two bytes at a time.



// common structure for FromAddress and ToAddress.
typedef struct tagFrameAddress
{
unsigned short Group;
unsigned short Site;
union UnitType Unit;
unsigned short SubUnit;
unsigned short Object;
} FrameAddress;

We do this differently since about 1998 in C++ (or even before?) :p



struct FrameAddress
{
unsigned short Group;
unsigned short Site;
union UnitType Unit;
unsigned short SubUnit;
unsigned short Object;
};



Why read all of the relevant file data into buffer then keep the file open for a long time? If something throws 'in the middle' then you have a resource leak because you are not using RAII.