Function call not returning

SimpleThings · July 9, 2017, 9:51am

I want to send various data from different sensors (data types) through serial port. But I am having a problem that after some time my function call never returns.
I made sequence diagram to depict you better what I am trying to implement:

Message.h
http://textuploader.com/dkcvh

Message.cpp
http://textuploader.com/dkcvg

MessageEncoder.h
http://textuploader.com/dkcv7

MessageEncoder.cpp
http://textuploader.com/dkcv2

MessageSerialization.h
http://textuploader.com/dkcvu

MessageSerialization.cpp
http://textuploader.com/dkcn4

MessageProcessor.h
http://textuploader.com/dkcvn

MessageProcessor.cpp
http://textuploader.com/dkcvy

main.cpp
http://textuploader.com/dkcnr

I have checked MessageEncoder and MessageSerialzation methods and they work fine, problem is in MessageProcessor class. After MessageProcessor::processTransmitData method call in main, method returns normally, but after some time everything stops. I have figured out that the problem resides in last 3 lines of MessageProcessor::processTransmitData method

	delete encodedMessage;
	delete serializedBuffer;

Somehow program gets stuck after calling these delete functions. And I am almost sure it has something to do with memory allocation for encodedMessage and serializedBuffer pointers. serializedBuffer is allocated in MessageProcessor::processTransmitData method, and encodedMessage is allocated in MessageEncoder::encode method. I think that at some point, stackoverflow happens
you can see at the end of COM port at this picture that output gets stuck.

Robin2 · July 9, 2017, 10:06am

Am I correct to think your program is spread over those 9 files?

Sorry, I'm too lazy to read more than one (maybe 2 at a stretch). Can you make a simple example that illustrates the problem?

And your pictures are unreadable.

...R

SimpleThings · July 9, 2017, 10:20am

Robin2:
Am I correct to think your program is spread over those 9 files?

Sorry, I'm too lazy to read more than one (maybe 2 at a stretch). Can you make a simple example that illustrates the problem?

And your pictures are unreadable.

...R

Yes there are over 9 files, but maybe if you had opened them you would had realised that they are short files with at most 30 lines of code. I wanted to make it more logical to me and to put different functionalities in different classes.
You can to click on the picture to enlarge it.

Whandall · July 9, 2017, 10:22am

You somhow manage to corrupt the heap.

Deleting objects twice, overwriting memory via bad pointers or writing beyond array limits are some
of the common strategies to achieve that.

If you have a lot of heap activity you have many vulnerable spots (the control blocks in front of each
allocation) that are used unchecked in each new/delete.

I will not look at pictures of code scattered over 9 locations.

SimpleThings · July 9, 2017, 11:19am

You have to only check:
MessageProcessor.cpp
http://textuploader.com/dkcvy
I have already written that there lays a problem.

Robin2 · July 9, 2017, 11:23am

SimpleThings:
Yes there are over 9 files, but maybe if you had opened them ]

Like I said. I am lazy.

...R

SimpleThings · July 9, 2017, 11:39am

You do not have to open 1 file and look at it but yet you have time to argue with me why you do not want to look at them, I guess this is the wrong place to ask a question.

arduarn · July 9, 2017, 11:46am

Holy over-engineering Batman!

I've been a little more generous and looked at the files, but clickable links would have been nice.

void loop()
{
  retVal == messageProcessor.processTransmitData(0x01, 4, data);
...
}

Oops.

#define MESSAGE_PROCESSOR_NOK -1
const char* MessageSerialization::serialize(Message* serialMessage)
{
...
	serializedBuffer = new uint8_t[serializedBufferLen];
	if ( serializedBuffer == NULL )
	{
      return MESSAGE_SERIALIZATION_NOK;
	}
...
}
int8_t MessageProcessor::processTransmitData(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)
{
...
	serializedBuffer = this->messageSerialization->serialize(encodedMessage);
	if ( serializedBuffer == NULL )
	{
	  return MESSAGE_PROCESSOR_NOK;
	}
...
}

Returning MESSAGE_SERIALIZATION_NOK as an error pointer?

Message* MessageEncoder::encode(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)
{
	Message* encodedMessage = new Message(dataType, dataLength, dataBuffer);
	
	if ( encodedMessage == NULL )
	{
		return NULL;
	}
	
	return encodedMessage;
}

Why not simply:

Message* MessageEncoder::encode(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)
{
	return new Message(dataType, dataLength, dataBuffer);
}

Or bet yet, dump the MessageEncoder class altogether, at least until you have things working.

const char* Message::getDataBuffer()
{
...
	returnDataBuffer = new uint8_t[this->dataLength + 1];
...
	memset(&returnDataBuffer[this->dataLength + 1], '\0', 1);
...
}

You are writing a byte beyond the end of the allocation.
Better: returnDataBuffer[this->dataLength] = '\0'

const char* MessageSerialization::serialize(Message* serialMessage)
{
...
memcpy( &serializedBuffer[MESSAGE_DATA_START_INDEX], serialMessage->getDataBuffer(), serialMessage->getDataLength() );
...
}

Where do you delete the buffer allocated by serialMessage->getDataBuffer() ?

Robin2 · July 9, 2017, 12:21pm

SimpleThings:
You do not have to open 1 file and look at it but yet you have time to argue with me why you do not want to look at them, I guess this is the wrong place to ask a question.

When I ask a question here (or in any other Forum) I take a great deal of trouble to minimize the effort needed by people who are kind enough to give their time to help me. You philosophy may be different.

...R

SimpleThings · July 9, 2017, 12:35pm

First of all arduarn thank you for you answer.

arduarn:
Holy over-engineering Batman!

I've been a little more generous and looked at the files, but clickable links would have been nice.
void loop()

{
retVal == messageProcessor.processTransmitData(0x01, 4, data);
...
}

Oops.
Yeah copy pasted it and haven't even notice..., I fixed it now.

arduarn:

#define MESSAGE_PROCESSOR_NOK -1

const char* MessageSerialization::serialize(Message* serialMessage)
{
...
serializedBuffer = new uint8_t[serializedBufferLen];
if ( serializedBuffer == NULL )
{
return MESSAGE_SERIALIZATION_NOK;
}
...
}
int8_t MessageProcessor::processTransmitData(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)
{
...
serializedBuffer = this->messageSerialization->serialize(encodedMessage);
if ( serializedBuffer == NULL )
{
return MESSAGE_PROCESSOR_NOK;
}
...
}



Returning MESSAGE_SERIALIZATION_NOK as an error pointer?

I agree it's not very descriptive I changed it to

 if ( encodedMessage == NULL)
 {
  return MESSAGE_PROCESSOR_NULL_PTR;
 }
 
 serializedBuffer = this->messageSerialization->serialize(encodedMessage);
 if ( serializedBuffer == NULL )
 {
  return MESSAGE_PROCESSOR_NULL_PTR;
 }

Is that any better?

arduarn:

Message* MessageEncoder::encode(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)

{
Message* encodedMessage = new Message(dataType, dataLength, dataBuffer);

if ( encodedMessage == NULL )
{
return NULL;
}

return encodedMessage;
}

Why not simply:
Message* MessageEncoder::encode(uint8_t dataType, uint8_t dataLength, uint8_t* dataBuffer)
{
return new Message(dataType, dataLength, dataBuffer);
}
Or bet yet, dump the MessageEncoder class altogether, at least until you have things working.

I changed it the way you wrote, I simply forgot we can return objects/object pointers this way in C++ as we can do it in Java and C#

const char* Message::getDataBuffer()
{
...
 returnDataBuffer = new uint8_t[this->dataLength + 1];
...
 memset(&returnDataBuffer[this->dataLength + 1], '\0', 1);
...
}

You are writing a byte beyond the end of the allocation.
Better: returnDataBuffer[this->dataLength] = '\0'
[/quote]

Ye I forgot that indexes do not start from 1 but from 0.

arduarn:

const char* MessageSerialization::serialize(Message* serialMessage)

{
...
memcpy( &serializedBuffer[MESSAGE_DATA_START_INDEX], serialMessage->getDataBuffer(), serialMessage->getDataLength() );
...
}

Where do you delete the buffer allocated by serialMessage->getDataBuffer() ?
I changed it to
#include "MessageSerialization.h"
#include "Message.h"

const char* MessageSerialization::serialize(Message* serialMessage)
{
uint8_t* tempDataBuffer;
.
.
.
tempDataBuffer = serialMessage->getDataBuffer();
memcpy( &serializedBuffer[MESSAGE_DATA_START_INDEX], tempDataBuffer, serialMessage->getDataLength() );
delete tempDataBuffer;
.
.
.
}

Now it works like a charm, thank you so much, I really appriciate that you took your time to help me.

septillion · July 9, 2017, 12:40pm

SimpleThings:
Is that any better?

Not really, you still try to return -1 as a pointer... Where do you think it will point at?

And not C++11 has a better alternative for NULL in a pointer namely nullptr

SimpleThings · July 9, 2017, 12:41pm

sry for double post

SimpleThings · July 9, 2017, 12:47pm

Robin2:
When I ask a question here (or in any other Forum) I take a great deal of trouble to minimize the effort needed by people who are kind enough to give their time to help me. You philosophy may be different.

...R

I do not know what to tell you I tried my best. You had to look at 5 .cpp files ( with no of lines of code: 33 + 34 + 42 + 7 + 24), that's 140 lines of code. I tried to be as descriptive as much as possible in my first post.

septillion:
Not really, you still try to return -1 as a pointer... Where do you think it will point at?

And not C++11 has a better alternative for NULL in a pointer namely nullptr

I forgot that it returns const char*, not int8_t ... I guess I should have made it returning int8_t as return value and instead giving a pointer to a buffer as a function parameter?

septillion · July 9, 2017, 12:49pm

That's a possibility. Don't really know what you try to do. Can't you just return nullptr (/NULL) if it's an error or does that already have a different meaning?

SimpleThings · July 9, 2017, 1:09pm

septillion:
That's a possibility. Don't really know what you try to do. Can't you just return nullptr (/NULL) if it's an error or does that already have a different meaning?

No returning NULL does not have any different meaning, I listened to your advice and put nullptr as return value. I just thought maybe it would be better to add integer as return value, perhaps if I want to add some other functionalities in this function and perhaps there will be a need to return some other error codes.

septillion · July 9, 2017, 1:17pm

Things like that depend on your design. Advantage of returning nullptr in case of an error (or not needing to do anything) is that you only have to mess with a single (return) variable. Downside is you only have one error/inactive state (the nullptr) so you have no way of passing a description of for example the difference between error and just do nothing.

Topic		Replies	Views
delete call causes programm to hang Programming	19	2601	April 1, 2021
Pointer returns garbage Programming	21	208	May 18, 2026
Rückgabe eines Char-Arrays aus einer Funktion Deutsch	15	374	April 9, 2025
Infinite rebooting Programming	15	225	January 2, 2025
Curious about the best way of returning arrays from a function [solved] Programming	54	2045	January 20, 2023

Function call not returning

Related topics