Bug report - class Print, function size_t print(const __FlashStringHelper *);

IDE version 1.6.12

Not sure where you are supposed to report this sort of thing, so here is as good as a place as any.

There appears to be a bug in the function size_t Print::print(const __FlashStringHelper *) such that it 'prints' only the first charter of the flash string.

I am actually calling the print function on an object of the derived class below.
There is no implementation of function print(...) in this class so I assume I am calling the print function in the base class.

class UDP is derived from class Stream and class stream is derived from class Print (all in Arduino\hardware\arduino\avr\cores\arduino)

My receiving windows application was actually receiving only the first character of the string.
But ss soon as I changed my arduino sketch code to use the size_t Print::write(const char *buffer, size_t size), my receiving Windows application received the full string.

class WiFiEspUDP : public UDP {
private:
  uint8_t _sock;  // socket ID for Wiz5100
  uint16_t _port; // local port to listen on
  
  
  uint16_t _remotePort;
  char _remoteHost[30];


public:
  WiFiEspUDP();  // Constructor

  virtual uint8_t begin(uint16_t);	// initialize, start listening on specified port. Returns 1 if successful, 0 if there are no sockets available to use
  virtual void stop();  // Finish with the UDP socket

  // Sending UDP packets

  // Start building up a packet to send to the remote host specific in ip and port
  // Returns 1 if successful, 0 if there was a problem with the supplied IP address or port
  virtual int beginPacket(IPAddress ip, uint16_t port);

  // Start building up a packet to send to the remote host specific in host and port
  // Returns 1 if successful, 0 if there was a problem resolving the hostname or port
  virtual int beginPacket(const char *host, uint16_t port);

  // Finish off this packet and send it
  // Returns 1 if the packet was sent successfully, 0 if there was an error
  virtual int endPacket();

  // Write a single byte into the packet
  virtual size_t write(uint8_t);

  // Write size bytes from buffer into the packet
  virtual size_t write(const uint8_t *buffer, size_t size);

  using Print::write;

  // Start processing the next available incoming packet
  // Returns the size of the packet in bytes, or 0 if no packets are available
  virtual int parsePacket();

  // Number of bytes remaining in the current packet
  virtual int available();

  // Read a single byte from the current packet
  virtual int read();

  // Read up to len bytes from the current packet and place them into buffer
  // Returns the number of bytes read, or 0 if none are available
  virtual int read(unsigned char* buffer, size_t len);

  // Read up to len characters from the current packet and place them into buffer
  // Returns the number of characters read, or 0 if none are available
  virtual int read(char* buffer, size_t len) { return read((unsigned char*)buffer, len); };

  // Return the next byte from the current packet without moving on to the next byte
  virtual int peek();

  virtual void flush();	// Finish reading the current packet

  // Return the IP address of the host who sent the current incoming packet
  virtual IPAddress remoteIP();

  // Return the port of the host who sent the current incoming packet
  virtual uint16_t remotePort();

  void getRemoteIP(IPAddress &IP);
  uint16_t getRemotePort();

  uint8_t getFirstSocket();


  friend class WiFiEspServer;
};

Post your code, or even better, a short example demonstrating this. I can only guess how you are using this class in your sketch. The issue is most likely in the code you have written.

I won't post every last bit of code - only the most relevant bits.

The following is where I am instigating the communication in my windows MFC application.

The user has entered a local IP address and hit a 'connect' button resulting in this function being called.

All I am doing is using my custom CIrrigationSocket class (derived from MFC's CSocket) to send the request "irrigation" to the specified local IP port. Then I wait for a response "irrigation" and if I get it then I know the deivce is one of my irrigation controllers.

See "m_Socket.Request("irrigation", strResp, m_strIPAddr, m_nPort)" The entire class is in the attached IrrigationSocket.cpp/.h

bool CIrrigationUploaderUDPDlg::TryConnect(LPCSTR lpszIPAddr)
{
 bool bResult = false;
 CString strResp, strIPAddr = m_strIPAddr, strNum, strMsg = "Could not connect to '" + m_strIPAddr + "'";
 UINT nPort = m_nPort;
 CProgressDlg dlg(GetParent());

 dlg.SetProgress(25, "Checking if '" + m_strIPAddr + "' is an irrigation controller.");
 if (m_Socket.Request("irrigation", strResp, m_strIPAddr, m_nPort))
 {
 if (strResp == "irrigation")
 {
 dlg.SetProgress(50, "Fetching name from '" + m_strIPAddr + "'.");
 if (m_Socket.Request("id", strResp, m_strIPAddr, m_nPort))
 {
 dlg.SetProgress(75, "Connection to '" + m_strIPAddr + "' was successful!");
 strNum.Format("%d", m_nPort);
 strMsg = "Successfully connected to '" + strResp + "' at '" + m_strIPAddr + "' on port " + strNum + "!";
 dlg.Close();
 m_staticStatus.SetWindowTextA(strMsg);
 ChangeTab(m_tabActivity.GetCurSel());
 bResult = true;
 }
 }
 }
 m_staticStatus.SetWindowTextA(strMsg);

 return bResult;
}

The attached SerialManager.cpp/.h is the Arduino code that receives and responds to the request.

To save you a bit of effort I have two pointers in the class: m_pUDPServer and m_pSerialHC05
I have arranged things so that only one of them will be non null at any given time.

If the serial data is coming in from the HC05 bluetooth modem then m_pSerialHC05 is non null, if the serial data is coming in via UDP then m_pUDPServer is non null.

The most relevant functions to look at are void processData(), bool writeWord(const __FlashStringHelper *strBuff), bool writeWord(const char *strBuff) and bool readWord(CString& strBuff)

Please note that I have since changed the implementation of "bool Print::writeWord(const char *strBuff)" and "bool writeWord(const __FlashStringHelper *strBuff)" to use "size_t write(const char *buffer, size_t size)".

There is no problem sending strings via this function.

But originally the implementation of "bool writeWord(const __FlashStringHelper *strBuff)" was using "size_t Print::print(const __FlashStringHelper *)" and with this my windows application was only seeing the first character of the string "irrigation" sent by my arduino code.

SerialManager.cpp (20.2 KB)

SerialManager.h (980 Bytes)

IrrigationSocket.cpp (4.69 KB)

IrrigationSocket.h (769 Bytes)

SerialManager.cpp (20.2 KB)

SerialManager.h (980 Bytes)

Please show the updated versions of the writeWord methods; the description only states that it takes an additional parameter but not how it's used.

And please show a typical usage example of those functions in a compilable Arduino sketch (as @pYro_65 asked).

Note
I'm not a C++ programmer

sterretje:
Please show the updated versions of the writeWord methods; the description only states that it takes an additional parameter but not how it's used.

And please show a typical usage example of those functions in a compilable Arduino sketch (as @pYro_65 asked).

Note
I'm not a C++ programmer

Provided are the current updated versions of those function. Rather below is what I had when I was getting only 'i' from "irrigation" at the windows socket end.

bool CSerialManager::writeWord(const __FlashStringHelper *strBuff)
{
  uint16_t nSize = strlen_P(strBuff) + 2;
  char *strDelimBuff = new char[nSize];
  
  memset(strDelimBuff, 0, nSize);
  strcpy_P(strDelimBuff, (PGM_P)strBuff);
  strDelimBuff[strlen(strDelimBuff)] = SERAIL_DELIM;
 
	if (m_pUDPServer)
	{
		m_pUDPServer->beginPacket(m_pUDPServer->remoteIP(), m_pUDPServer->remotePort());
		nSize = m_pUDPServer->print(strDelimBuff);
    debug.log(nSize, false);
    debug.log(F(" bytes written to UDP port."));
		m_pUDPServer->endPacket();
	}
	else if (m_pSerialHC05)
	{
		m_pSerialHC05->print(strDelimBuff);
	}
 delete strDelimBuff;
}
CSerialManager::CSerialManager(CWifiManager &WifiManager, CProgram &program): m_WifiManager(WifiManager), m_program(program)

Only Microsoft employees are dumb enough to need to be constantly reminded that something is a Class. You don't work for Microsoft, do you?

bool CSerialManager::writeWord(const __FlashStringHelper *strBuff)
{
  uint16_t nSize = strlen_P((PGM_P)strBuff);
  char *strNormalBuff = new char[nSize];
  
  memset(strNormalBuff, 0, nSize);
  strcpy_P(strNormalBuff, (PGM_P)strBuff);
  writeWord(strNormalBuff);
  delete strNormalBuff;
}

Are you POSITIVE that strlen_P() is getting the proper length of the string?

The Print::print() method that takes a F() string is:

size_t Print::print(const __FlashStringHelper *ifsh)
{
  PGM_P p = reinterpret_cast<PGM_P>(ifsh);
  size_t n = 0;
  while (1) {
    unsigned char c = pgm_read_byte(p++);
    if (c == 0) break;
    if (write(c)) n++;
    else break;
  }
  return n;
}

Notice that is does NOT attempt to determine, a-priori, the length of the string.

I just noticed I have not changed out my other call to Print::print(...)

My assumption is that Print::print(...) would keep printing chars until it finds a null terminator. That is what all other string related functions do.

Why would you bother creating a function with string parameter that only does something with the first character?

My assumption is that Print::print(...) would keep printing chars until it finds a null terminator. That is what all other string related functions do.

It does, as illustrated in the Print::print() method that I posted.

Why would you bother creating a function with string parameter that only does something with the first character?

Who did that? It is YOUR function for dealing with a Flash string that is broken, NOT the one in the Print class.

How is my flash string or the function that deals with it broken?

else if (strWord.indexOf(F("irrigation")) > -1)
 writeWord(F("irrigation"));

"Are you POSITIVE that strlen_P() is getting the proper length of the string?"
As far as I am aware that is the correct way to use strlen_P(...) from the examples I previsouly looked at. I have used it like that plenty of times before with no problems. If I am not using it correctly then can you point out how it is incorrect.

If I am not using it correctly then can you point out how it is incorrect.

I'm not saying that it is, or is not, correct. I'm asking you to prove that it is. Print the length that you determine. If it is correct, fine. If not, well, then we have more data that we did before.

I can't see the purpose of having a function that takes a string stored in flash, and then copying the string into SRAM. The whole point of the F() macro, and __FlashStringHelper is to prevent that from happening.

I just noticed that I did not have the original code above quite right. This was how it was:

bool CSerialManager::writeWord(const __FlashStringHelper *strBuff)
{
  uint16_t nSize = strlen_P((PGM_P)strBuff);
  char *strNormalBuff = new char[nSize + 1];
  
  memset(strNormalBuff, 0, nSize);
  strcpy_P(strNormalBuff, (PGM_P)strBuff);
  strcat(strNormalBuff, SERIAL_DELIM);
  if (pUDPServer)
  {
    m_pUDPServer->beginPacket(m_pUDPServer->remoteIP(), m_pUDPServer->remotePort());
    m_pUDPServer->print(strNormalBuff);
    m_pUDPServer->endPacket();
    
  }
  else if (m_pSerialHC05)
  {
    pSerialHC05->print(strNormalBuff);
  }
}

Then I had virtually the same code in bool CSerialManager::writeWord(const char* strBuff)
except using the normal string functions strlen(...) etc

I.E. My original versions of those functions were following the attached example for sending UDP data

The point of these functions was to standardize the use of the tilde character as a delimiter in the data streams and to check for incomplete data.

Rather than having to explicitly add the tilde character in my literal strings etc which can easily lead to accidental omissions.

UdpSendReceive.ino (2.87 KB)

The point of these functions was to standardize the use of the tilde character as a delimiter in the data streams and to check for incomplete data.

I still don't see the need to copy the data from Flash memory to SRAM. The whole point of the __FlashStringHelper and F() macro and type is to prevent that from happening. There is no reason to do that in order to append a ~ to the data being sent. Just send the data, one character at a time, like Print::print() does, until it has all been sent, and then send the ~.

beginPacket() just initializes a buffer, while the UDP print method writes to the buffer. The buffer is not actually sent until endPacket() is called, so having three copies of the data (the one in Flash, the one you make, and the one in the UDP class) seems completely unnecessary.

Finally, I still have seen no proof that strlen_P() actually returns correct data.

PaulS:
I still don't see the need to copy the data from Flash memory to SRAM. The whole point of the __FlashStringHelper and F() macro and type is to prevent that from happening. There is no reason to do that in order to append a ~ to the data being sent. Just send the data, one character at a time, like Print::print() does, until it has all been sent, and then send the ~.

beginPacket() just initializes a buffer, while the UDP print method writes to the buffer. The buffer is not actually sent until endPacket() is called, so having three copies of the data (the one in Flash, the one you make, and the one in the UDP class) seems completely unnecessary.

Finally, I still have seen no proof that strlen_P() actually returns correct data.

Well was doing what you suggested also originally before I was strcat'ing all into one string. I arbitrarily decided that sending it all in one string and one call to print(...) or what ever looked nicer. Perhaps I will restore it to that method if you think is makes much of a difference.

But I don't understand in what the circumstances strlen_P(...) would fail to return the correct length of the flash string passed in as a parameter?

But I don't understand in what the circumstances strlen_P(...) would fail to return the correct length of the flash string passed in as a parameter?

I don't know that there are any. What I DO see is that Print::print() does not use that function to determine how much data to output. It simply walks through Flash memory, dealing with one character at a time, until it encounters a NULL.

I arbitrarily decided that sending it all in one string and one call to print(...) or what ever looked nicer.

The Arduino doesn't care how "nice" your code looks. What it cares about is the correctness of your code.

PaulS:
The Arduino doesn't care how "nice" your code looks. What it cares about is the correctness of your code.

Well may be not, but I care.

Although in hindsight, given that the mega is 16MHz and the pc is 2.3GHz or what ever, I may be introducing some noticeable delays by duplicating the data before sending.

Not right now because I am in the middle of something.

But I will put the call the Print::print(...) back and see if I can reproduce the error....and check the output of strlen_P(...)

So far, every time I've seen the Print class only printing the first character it is a problem with the underlying virtual function.
Return values matter.
The Print class looks at return values and will stop sending output if it sees a return status indicating an issue.

In older versions of the IDEs, Print didn't look at the return value from the called virtual write() function. Now it does, so older libraries that used Print class might have been accidentally working even though they were returning improper/invalid status values back to Print. Any library write() function that is returning improper values back to Print will likely break on newer IDEs now that the return values are examined.

--- bill

In your function:

bool CSerialManager::writeWord(const __FlashStringHelper *strBuff)
{
  uint16_t nSize = strlen_P((PGM_P)strBuff);
  char *strNormalBuff = new char[nSize + 1];
  
  memset(strNormalBuff, 0, nSize);
  strcpy_P(strNormalBuff, (PGM_P)strBuff);
  strcat(strNormalBuff, SERIAL_DELIM);
  if (pUDPServer)
  {
    m_pUDPServer->beginPacket(m_pUDPServer->remoteIP(), m_pUDPServer->remotePort());
    m_pUDPServer->print(strNormalBuff);
    m_pUDPServer->endPacket();
    
  }
  else if (m_pSerialHC05)
  {
    pSerialHC05->print(strNormalBuff);
  }
}

This line:

char *strNormalBuff = new char[nSize + 1];

shoudn't be:

char *strNormalBuff = new char[nSize + 2];

because of the SERIAL_DELIM?

Why are you using new and forgetting delete if you can do this:

char strNormalBuff[nSize + 2];

instead of:

char *strNormalBuff = new char[nSize + 2];

This block:

  uint16_t nSize = strlen_P((PGM_P)strBuff);
  char *strNormalBuff = new char[nSize + 2];
  
  memset(strNormalBuff, 0, nSize);
  strcpy_P(strNormalBuff, (PGM_P)strBuff);
  strcat(strNormalBuff, SERIAL_DELIM);

Could be written as:

  char strNormalBuff[strlen_P((PGM_P)strBuff) + 2]; // +1 for null-character and +1 for SERIAL_DELIM
  strcpy_P(strNormalBuff,(PGM_P)strBuff); //copies including a null-character
  strcat(strNormalBuff, SERIAL_DELIM); //concatenates including a null-character

I am deleting the pointers before the end of the functions - I probably just omitted the lines of code in cutting and pasting the code. I am very aware of memory leakage.

I re-instated the Print::print(...) function calls and the problems re-occurred at the windows app end, although the characters I am receiving are different to last time.

So I am thinking it is not a bug in Serial:print(...) but rather a serial data timing problem between arduino and windows. Windows is reading the serial data too fast and arduino is not sending it fast enough via Print::print(...)

However Print::write(buff, size) seems to work - no timing problems for what ever reason.

  uint16_t nSize = strlen_P((PGM_P)strBuff);
  char *strNormalBuff = new char[nSize];
...
  strcpy_P(strNormalBuff, (PGM_P)strBuff);

Oops.

From reply #17 that appears to be a common mistake.