Serial communication
-
@Christian-Ehrlicher @JonB Thank you for yours suggestions.
I have implemented your comments,suggestions, and now code look like:
#include "mydevice.h" MyDevice::MyDevice(QObject *parent) : QObject{parent} { serial = new QSerialPort(); serial->setPortName("COM9"); serial->setBaudRate(QSerialPort::Baud9600); serial->setDataBits(QSerialPort::Data8); serial->setParity(QSerialPort::EvenParity); serial->open(QIODevice::ReadWrite); connect(serial, &QSerialPort::readyRead, this, &MyDevice::serialRecive); connect(&timerResponse, &QTimer::timeout, this, [&](){ m_timeoutResponse = true; timerResponse.stop(); qDebug()<<"Timeout response";} ); connect(&timerState, &QTimer::timeout, this, &MyDevice::stateMachine); if(!serial->isOpen()){ qInfo() << "Serial port status: " << serial->isOpen(); }else{ //run state machine! m_state = State::state::STATUS_REQUEST; timerState.start(100); } } void MyDevice::serialRecive() { m_request_part.append(serial->readAll()); qDebug() << "Partially received: " << m_request_part; while(!messageComplete(m_request_part)) { return; } bool crcValid=validateResponse(m_request_part); if(!crcValid){ qDebug()<<"Crc error"; m_state = State::state::STATUS_REQUEST; return; } timerResponse.stop(); m_request = m_request_part; qDebug()<<"Correct response: " << m_request; } void MyDevice::stateMachine() { if (m_request.count() > 3) m_responseStatus = static_cast<Response::Status>(m_request.mid(2,1).toInt(nullptr, 16)); else m_responseStatus = Response::Status::UNKNOWN; switch (m_state) { case State::state::STATUS_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::POLL,m_data)); m_state = State::state::STATUS_RESPONSE; break; case State::state::STATUS_RESPONSE: if(m_responseStatus == Response::Status::POWER_UP) m_state = State::state::VERSION_REQUEST; if(m_responseStatus == Response::Status::POWER_UP_WITH_BILL_IN_ACCEPTOR) m_state = State::state::RESET_REQUEST; if(m_responseStatus == Response::Status::POWER_UP_WITH_BILL_IN_STACKER) m_state = State::state::RESET_REQUEST; if(m_responseStatus == Response::Status::INITIALIZE) m_state = State::state::EN_DIS_REQUEST; if(m_responseStatus == Response::Status::ENABLE) m_state = State::state::STATUS_REQUEST; if(m_responseStatus == Response::Status::ACCEPTING) m_state = State::state::STATUS_REQUEST; if(m_responseStatus == Response::Status::ESCROW) m_state = State::state::STACK1_REQUEST; if(m_responseStatus == Response::Status::STACKING) m_state = State::state::STATUS_REQUEST; if(m_responseStatus == Response::Status::STACKED) m_state = State::state::STATUS_REQUEST; if(m_responseStatus == Response::Status::VALID) m_state = State::state::ACK_REQUEST; if(m_responseStatus == Response::Status::REJECTING) m_state = State::state::STATUS_REQUEST; if(m_responseStatus == Response::Status::JAM_IN_ACCEPTOR) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::VERSION_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::GET_VERSION,m_data)); m_state = State::state::VERSION_RESPONSE; break; case State::state::VERSION_RESPONSE: if(m_responseStatus == Response::Status::STAT_VERSION) m_state = State::state::RESET_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::RESET_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::RESET,m_data)); m_state = State::state::RESET_RESPONSE; break; case State::state::RESET_RESPONSE: if(m_responseStatus == Response::Status::ACK) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::EN_DIS_REQUEST: timerResponse.start(m_timeout); clearBuffers(); m_data = QByteArray("\xff\xff",2); serial->write(createMessage(Commands::deviceCommand::ENABLE,m_data)); m_state = State::state::EN_DIS_RESPONSE; break; case State::state::EN_DIS_RESPONSE: if(m_responseStatus == Response::Status::STAT_EN_DIS) m_state = State::state::SECURITY_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::SECURITY_REQUEST: timerResponse.start(m_timeout); clearBuffers(); m_data = QByteArray("\xff\xff",2); serial->write(createMessage(Commands::deviceCommand::SECURITY,m_data)); m_state = State::state::SECURITY_REQUEST; break; case State::state::SECURITY_RESPONSE: if(m_responseStatus == Response::Status::CMD_ENABLE_SECURITY) m_state = State::state::OPTIONAL_FUN_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::OPTIONAL_FUN_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::OPTIONAL,m_data)); m_state = State::state::OPTIONAL_FUN_RESPONSE; break; case State::state::OPTIONAL_FUN_RESPONSE: if(m_responseStatus == Response::Status::CMD_OPTIONAL) m_state = State::state::INHIBIT_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::INHIBIT_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::INHIBIT,m_data)); m_state = State::state::INHIBIT_RESPONSE; break; case State::state::INHIBIT_RESPONSE: if(m_responseStatus == Response::Status::CMD_INHIBIT) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::STACK1_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::STACK1,m_data)); m_state = State::state::STACK1_RESPONSE; break; case State::state::STACK1_RESPONSE: if(m_responseStatus == Response::Status::ACK) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::STACK2_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::STACK2,m_data)); m_state = State::state::STACK2_RESPONSE; break; case State::state::STACK2_RESPONSE: if(m_responseStatus == Response::Status::ACK) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; case State::state::ACK_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::ACK,m_data)); m_state = State::state::STATUS_REQUEST; break; case State::state::RETURN_REQUEST: timerResponse.start(m_timeout); clearBuffers(); serial->write(createMessage(Commands::deviceCommand::RETURN,m_data)); m_state = State::state::RETURN_RESPONSE; break; case State::state::RETURN_RESPONSE: if(m_responseStatus == Response::Status::ACK) m_state = State::state::STATUS_REQUEST; if(m_timeoutResponse) { m_timeoutResponse = false; m_state = State::state::STATUS_REQUEST; } break; default: break; } } void MyDevice::clearBuffers() { m_request.clear(); m_data.clear(); m_request_part.clear(); } bool MyDevice::messageComplete(const QByteArray &data) const { if(data.size()>=3){ quint8 length=(unsigned char)data[1]; if(length==data.length()) return true; } return false; } bool MyDevice::validateResponse(QByteArray data) { if(data.size()<3) return false; QByteArray crc=data.right(2); std::reverse(crc.begin(),crc.end()); bool ok=false; quint16 messageCRC=crc.toHex().toUInt(&ok,16); if(!ok) return false; data.chop(2); return (messageCRC==crc16(data)); } QByteArray MyDevice::createMessage(const Commands::deviceCommand &cmd, const QByteArray &data) { QByteArray message; message.append(0xFC); if(data.count() > 0) { message.append(0x05+data.count()); } else{ message.append(0x05); } message.append((char)cmd); if(data.count() > 0) { for (int i= 0 ; i< data.count() ; i++) message.append(data[i]); } quint16 crc=crc16(message); QByteArray crcData=QByteArray((char *)&crc,2); message.append(crcData); return message; } quint16 MyDevice::crc16(const QByteArray &data) { unsigned int CRC; unsigned char j; CRC = 0; for(int i=0; i < data.size(); i++) { CRC ^= (unsigned char)data[i]; for(j=0; j < 8; j++) { if(CRC & 0x0001) {CRC >>= 1; CRC ^= POLYNOMIAL;} else CRC >>= 1; } } return CRC; }
I would be rally gratefull for more suggestions.
@Damian7546 said in Serial communication:
while(!messageComplete(m_request_part)) {
return;
}Why do you have a loop here if you anyway do a return?
-
@Damian7546 said in Serial communication:
while(!messageComplete(m_request_part)) {
return;
}Why do you have a loop here if you anyway do a return?
@jsulm Better?
if(!messageComplete(m_request_part)) { return; }
-
@jsulm Better?
if(!messageComplete(m_request_part)) { return; }
@Damian7546 More readable/clear :)
-
@Damian7546 More readable/clear :)
From my pov the parsing is incorrect as there may be more than one message in the buffer so the second (or even the first part of the second) is thrown away.
-
From my pov the parsing is incorrect as there may be more than one message in the buffer so the second (or even the first part of the second) is thrown away.
@Christian-Ehrlicher so how to fix it?
-
@Christian-Ehrlicher so how to fix it?
@Damian7546
Remove only what is used for a message from the buffer, leaving any further unused bytes there might be for appending to create next "message". -
@Damian7546 Something like this. But better don't use c-style casts:
m_responseStatus = static_cast<Response::Status>(m_request.mid(2,1).toInt(nullptr, 16));
@jsulm
This conversion doesn't work.class Response { public: enum class Status : quint8{ DISABLE = 0x1A, }; Response(); };
In
m_responseStatus
variable I have0x1A
value, but below statement is not equal:m_responseStatus = static_cast<Response::Status>(m_request.mid(2,1).toInt(nullptr, 16)); qDebug() << "m_responseStatus: " << m_request.mid(2,1); if(Response::Status::DISABLE == m_responseStatus) qDebug() << "ok convert"; else qDebug() << "bad convert";
Problem is in
toInt
conversion:qDebug() << "m_responseStatus: " << m_request.mid(2,1); qDebug() << "m_responseStatus: " << m_request.mid(2,1).toInt(nullptr, 16);
result:
m_responseStatus: "\x1A"
m_responseStatus: 0
-
@jsulm
This conversion doesn't work.class Response { public: enum class Status : quint8{ DISABLE = 0x1A, }; Response(); };
In
m_responseStatus
variable I have0x1A
value, but below statement is not equal:m_responseStatus = static_cast<Response::Status>(m_request.mid(2,1).toInt(nullptr, 16)); qDebug() << "m_responseStatus: " << m_request.mid(2,1); if(Response::Status::DISABLE == m_responseStatus) qDebug() << "ok convert"; else qDebug() << "bad convert";
Problem is in
toInt
conversion:qDebug() << "m_responseStatus: " << m_request.mid(2,1); qDebug() << "m_responseStatus: " << m_request.mid(2,1).toInt(nullptr, 16);
result:
m_responseStatus: "\x1A"
m_responseStatus: 0
@Damian7546 said in Serial communication:
In m_responseStatus variable I have 0x1A value, but below statement is not equal:
Sorry, I don't believe that. Start with:
qDebug() << int(m_responseStatus) << int(Response::Status::DISABLE) << 0x1A;
[I assume
int()
is good enough here to test. You can gostatic_cast<int>(...)
if necessary.] -
@Damian7546 said in Serial communication:
In m_responseStatus variable I have 0x1A value, but below statement is not equal:
Sorry, I don't believe that. Start with:
qDebug() << int(m_responseStatus) << int(Response::Status::DISABLE) << 0x1A;
[I assume
int()
is good enough here to test. You can gostatic_cast<int>(...)
if necessary.]@JonB said in Serial communication:
Sorry, I don't believe that. Start with:
qDebug() << int(m_responseStatus) << int(Response::Status::DISABLE) << 0x1A;Result:
0 26 26
-
@JonB said in Serial communication:
Sorry, I don't believe that. Start with:
qDebug() << int(m_responseStatus) << int(Response::Status::DISABLE) << 0x1A;Result:
0 26 26
@Damian7546
So you have your answer from the first 2 outputs.0 != 26
, the two values being compared are indeed not equal/the same.Behaviour is quite correct given your
m_request.mid(2,1).toInt(nullptr, 16);
, which (QByteArray::toInt()) is quite incorrect to use on your byte array containing a byte with value\x16
(26).toInt()
is not for use in your case, as you do not have a string representation of an integer to convert. -
@Damian7546
So you have your answer from the first 2 outputs.0 != 26
, the two values being compared are indeed not equal/the same.Behaviour is quite correct given your
m_request.mid(2,1).toInt(nullptr, 16);
, which (QByteArray::toInt()) is quite incorrect to use on your byte array containing a byte with value\x16
(26).toInt()
is not for use in your case, as you do not have a string representation of an integer to convert.@JonB So ho to compare
Response::Status::DISABLE
to byte in QByteArraym_request[2]
? -
@JonB So ho to compare
Response::Status::DISABLE
to byte in QByteArraym_request[2]
?@Damian7546
Exactly as you have just written it!qDebug() << Response::Status::DISABLE << m_request[2];
[You may have to do casting.] Sightly better is to use
m_request.at(2)
in place ofm_request[2]
here, but that is a detail and not related to your issue. You want to inspect the actual bytes in theQByteArray
, not callQByteArray::toInt()
. -
@Damian7546
Exactly as you have just written it!qDebug() << Response::Status::DISABLE << m_request[2];
[You may have to do casting.] Sightly better is to use
m_request.at(2)
in place ofm_request[2]
here, but that is a detail and not related to your issue. You want to inspect the actual bytes in theQByteArray
, not callQByteArray::toInt()
. -
@JonB casting is highly suggested, as
Response::Status
is an uint_8t and at() returns a char, aka int8_t@J-Hilk
Yes, but what I meant is I don't have anything available to test what compiler says as I write answers. So that detail is left to OP :) Since the OP asks about comparing (with==
) one should be able to compare aunit_8t
against achar
/int8_t
, I think, using C++ automatic expression promotion without bothering to cast/convert, I think? But maybe not/warning forenum
value against integer?The important thing is no
QByteArray::toInt()
here! -
@J-Hilk
Yes, but what I meant is I don't have anything available to test what compiler says as I write answers. So that detail is left to OP :) Since the OP asks about comparing (with==
) one should be able to compare aunit_8t
against achar
/int8_t
, I think, using C++ automatic expression promotion without bothering to cast/convert, I think? But maybe not/warning forenum
value against integer?The important thing is no
QByteArray::toInt()
here!@JonB It doesn't work:
qDebug() << static_cast<int>(Response::Status::DISABLE) << m_request.at(2);
Result:
ASSERT: "uint(i) < uint(size())" in file C:/Qt/5.15.2/mingw81_32/include/QtCore/qbytearray.h, line 500
And this way:
qDebug() << static_cast<int>(Response::Status::DISABLE) << m_request[2];
Result:
Where :
m_request response: "\xFC\x05\x1A\xF4\xE8"
-
@JonB It doesn't work:
qDebug() << static_cast<int>(Response::Status::DISABLE) << m_request.at(2);
Result:
ASSERT: "uint(i) < uint(size())" in file C:/Qt/5.15.2/mingw81_32/include/QtCore/qbytearray.h, line 500
And this way:
qDebug() << static_cast<int>(Response::Status::DISABLE) << m_request[2];
Result:
Where :
m_request response: "\xFC\x05\x1A\xF4\xE8"
@Damian7546
How difficult to try:qDebug() << static_cast<int>(Response::Status::DISABLE) << static_cast<int>(m_request[2]); // or qDebug() << static_cast<int>(Response::Status::DISABLE) << static_cast<int>(m_request.at(2));
? I leave you to resolve the C++ to your satisfaction.
-
@J-Hilk
Yes, but what I meant is I don't have anything available to test what compiler says as I write answers. So that detail is left to OP :) Since the OP asks about comparing (with==
) one should be able to compare aunit_8t
against achar
/int8_t
, I think, using C++ automatic expression promotion without bothering to cast/convert, I think? But maybe not/warning forenum
value against integer?The important thing is no
QByteArray::toInt()
here!@JonB said in Serial communication:
one should be able to compare a unit_8t against a char/int8_t, I think, using C++ automatic expression promotion without bothering to cast/convert, I think?
nono, comparing different types via == or != will never promote any side "automatically"
you can get something like that fi you wrap it in a function call or something but.
But comparing uint8_t and int_8 will only work correctly if both values are between 0 and 128.
-
@Damian7546
How difficult to try:qDebug() << static_cast<int>(Response::Status::DISABLE) << static_cast<int>(m_request[2]); // or qDebug() << static_cast<int>(Response::Status::DISABLE) << static_cast<int>(m_request.at(2));
? I leave you to resolve the C++ to your satisfaction.
@JonB Sill of topic.
I have variable:Response::Status m_responseStatus
.
How properly assign byte form QByteArray to m_responseStatus? -
@JonB Sill of topic.
I have variable:Response::Status m_responseStatus
.
How properly assign byte form QByteArray to m_responseStatus?@Damian7546 you were originally pretty spot on:
static_cast<Response::Status>(m_request.at(0));
-
@JonB said in Serial communication:
one should be able to compare a unit_8t against a char/int8_t, I think, using C++ automatic expression promotion without bothering to cast/convert, I think?
nono, comparing different types via == or != will never promote any side "automatically"
you can get something like that fi you wrap it in a function call or something but.
But comparing uint8_t and int_8 will only work correctly if both values are between 0 and 128.
@J-Hilk said in Serial communication:
nono, comparing different types via == or != will never promote any side "automatically"
Umm, C or C++
char c('A'); if (c == 65) {} example.cpp Compiler returned: 0
From Godbolt. That's "automatically promoting"
char c
toint 65
, per C/C++ expression rules. Has done since K&R... ?
https://en.cppreference.com/w/cpp/language/implicit_conversion, e.g. Integral conversions topic.