-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
freing _frame too early (writeMultipleCoils sets wrong values) #53
Comments
You are right, this is a severe issue if your system (e.g. esp32) is capable of running multiple processes and uses dynamic memory more than a small embedded with no other processes running. That may be the reason why not all users do have reported problems. I have got the same problems with this issue and needed to change the code. But your way to change it is in my eyes not the best way to correct this issue. By placing the whole response section behind the register writing processes (btw. this issue is also present to write multiple coils) you are not acting according to Modbus error compliance: When the malloc() function in allocating the frame for the answer does give an error back you will answer the function call from the Modbus master with an error code although you have changed the registers successfully according to the write function call. To avoid this scenario there is no other way than using a second frame buffer for input. So I defined an "_iframe" in Modbus.h:
` |
In this function for example _frame is freed and after that frame[6+i] is read. But frame actually points to _frame:
void Modbus::writeMultipleCoils(byte* frame,word startreg, word numoutputs, byte bytecount)
See code changes below:
Original code:
//Clean frame buffer
free(_frame);
_len = 5;
_frame = (byte *) malloc(_len);
if (!_frame) {
this->exceptionResponse(MB_FC_WRITE_COILS, MB_EX_SLAVE_FAILURE);
return;
}
_frame[0] = MB_FC_WRITE_COILS;
_frame[1] = startreg >> 8;
_frame[2] = startreg & 0x00FF;
_frame[3] = numoutputs >> 8;
_frame[4] = numoutputs & 0x00FF;
Changed code:
See issue #35 as well.
The text was updated successfully, but these errors were encountered: