Added buffer checks to command interfaces

Limits risk over overflowing thread ram during command flooding

Author: Ultrawipf

Firmware/FFBoard/Inc/CDCcomm.h

@@ -17,6 +17,7 @@ class CDCcomm {
 
 	static uint16_t cdcSend(std::string* reply,uint8_t itf);
 	static void cdcFinished(uint8_t itf = 0);
+	static uint32_t remainingData(uint8_t itf = 0);
 
 private:
 	static bool usb_busy_retry;

Firmware/FFBoard/Inc/CmdParser.h

@@ -28,13 +28,14 @@ class CmdParser {
 	void clear();
 	bool add(char* Buf, uint32_t *Len);
 	bool parse(std::vector<ParsedCommand>& commands);
-	uint32_t bufferCapacity();
+	int32_t bufferCapacity();
 
 	void setClearBufferTimeout(uint32_t timeout);
 
 private:
 	std::string buffer;
-	uint32_t reservedBuffer = 0;
+	int32_t reservedBuffer = 100;
+	int32_t bufferMaxCapacity = 512;
 
 	uint32_t clearBufferTimeout = 0;
 	uint32_t lastAddTime = 0;

Firmware/FFBoard/Inc/CommandInterface.h

@@ -32,6 +32,7 @@ class CommandInterface {
 	virtual bool hasNewCommands();
 	static void broadcastCommandReplyAsync(std::vector<CommandReply>& reply,CommandHandler* handler, uint32_t cmdId,CMDtype type);
 	virtual void sendReplies(std::vector<CommandResult>& results,CommandInterface* originalInterface); // All commands from batch done
+	virtual bool readyToSend();
 protected:
 	bool parserReady = false;
 };
@@ -43,6 +44,7 @@ class StringCommandInterface : public CommandInterface{
 public:
 	StringCommandInterface(uint32_t reservedBuffer = 16) : parser(CmdParser(reservedBuffer)){}
 	bool addBuf(char* Buf, uint32_t *Len);
+	uint32_t bufferCapacity();
 	bool getNewCommands(std::vector<ParsedCommand>& commands) override;
 	static void formatReply(std::string& reply,const std::vector<CommandResult>& results,const bool formatWriteAsRead=false);
 	static std::string formatOriginalCommandFromResult(const ParsedCommand& originalCommand,CommandHandler* commandHandler,const bool formatWriteAsRead=false);
@@ -64,7 +66,7 @@ class CDC_CommandInterface : public StringCommandInterface{
 
 	void sendReplies(std::vector<CommandResult>& results,CommandInterface* originalInterface) override;
 	const std::string getHelpstring(){return "CDC interface. " + StringCommandInterface::getHelpstring();};
-
+	bool readyToSend() override;
 private:
 	bool enableBroadcastFromOtherInterfaces = true;
 	const uint32_t parserTimeout = 2000;

Firmware/FFBoard/Inc/HidCommandInterface.h

@@ -47,15 +47,16 @@ class HID_CommandInterface : public CommandInterface, public cpp_freertos::Threa
 	bool sendHidCmd(HID_CMD_Data_t* data);
 	void queueReplyValues(CommandReply& reply,ParsedCommand& command);
 	void transferComplete(uint8_t itf, uint8_t const* report, uint8_t len);
-
+	bool readyToSend();
 	void Run();
 
 private:
 	std::vector<ParsedCommand> commands;
 	std::vector<HID_CMD_Data_t> outBuffer;
 	bool enableBroadcastFromOtherInterfaces = true; // TODO make configurable via command
 	static cpp_freertos::BinarySemaphore threadSem;
-
+	const uint32_t maxQueuedReplies = 50;
+	const uint32_t maxQueuedRepliesBroadcast = 10; // Must be smaller than maxQueuedReplies
 
 };
 

Firmware/FFBoard/Inc/constants.h

@@ -10,7 +10,7 @@
 
 
 
-static const uint8_t SW_VERSION_INT[3] = {1,6,1}; // Version as array. 8 bit each!
+static const uint8_t SW_VERSION_INT[3] = {1,6,2}; // Version as array. 8 bit each!
 
 #define MAX_AXIS 2 // ONLY USE 2 for now else screws HID Reports
 

Firmware/FFBoard/Src/CDCcomm.cpp

@@ -38,6 +38,10 @@ void CDCcomm::cdcFinished(uint8_t itf){
 	}
 }
 
+uint32_t CDCcomm::remainingData(uint8_t itf){
+	return CDCcomm::remainingStrs[itf].size();
+}
+
 /**
  * Sends a string via CDC
  * If not everything can be sent it will be buffered for later in a new string

Firmware/FFBoard/Src/CmdParser.cpp

@@ -50,8 +50,8 @@ void CmdParser::setClearBufferTimeout(uint32_t timeout){
 	this->clearBufferTimeout = timeout;
 }
 
-uint32_t CmdParser::bufferCapacity(){
-	return buffer.capacity() - buffer.size();
+int32_t CmdParser::bufferCapacity(){
+	return std::max<int32_t>(bufferMaxCapacity - buffer.size(),0);
 }
 
 

Firmware/FFBoard/Src/CommandInterface.cpp

@@ -63,6 +63,14 @@ bool CommandInterface::hasNewCommands(){
 	return parserReady;
 }
 
+/**
+ * Returns true if it can send replies.
+ * False if the command thread should wait until previous replies are processed
+ */
+bool CommandInterface::readyToSend(){
+	return true;
+}
+
 
 /*
  *
@@ -88,6 +96,10 @@ bool StringCommandInterface::addBuf(char* Buf, uint32_t *Len){
 	return res;
 }
 
+uint32_t StringCommandInterface::bufferCapacity(){
+	return parser.bufferCapacity();
+}
+
 void StringCommandInterface::formatReply(std::string& reply,const std::vector<CommandResult>& results,const bool formatWriteAsRead){
 	//uint16_t lastId = 0xFFFF;
 	for(const CommandResult& result : results){ // All commands processed this batch
@@ -214,16 +226,24 @@ CDC_CommandInterface::~CDC_CommandInterface() {
 
 
 void CDC_CommandInterface::sendReplies(std::vector<CommandResult>& results,CommandInterface* originalInterface){
-	if( (!enableBroadcastFromOtherInterfaces && originalInterface != this) || !tud_ready() ){
+	if( (!enableBroadcastFromOtherInterfaces && originalInterface != this) ){
 		return;
 	}
+
 	std::string replystr;
 	replystr.reserve(100);
 	StringCommandInterface::formatReply(replystr,results, originalInterface != this && originalInterface != nullptr);
 	if(!replystr.empty())
 		CDCcomm::cdcSend(&replystr, 0);
 }
 
+/**
+ * Ready to send if there is no data in the backup buffer of the cdc port
+ */
+bool CDC_CommandInterface::readyToSend(){
+	return CDCcomm::remainingData(0) == 0;
+}
+
 
 
 /*************************
@@ -233,7 +253,6 @@ void CDC_CommandInterface::sendReplies(std::vector<CommandResult>& results,Comma
  *
  */
 
-
 extern UARTPort external_uart; // defined in cpp_target_config.cpp
 UART_CommandInterface::UART_CommandInterface(uint32_t baud) : StringCommandInterface(128), UARTDevice(external_uart),Thread("UARTCMD", 256, 36), baud(baud){ //
 	uartconfig = uartport->getConfig();
@@ -280,7 +299,7 @@ void UART_CommandInterface::Run(){
 //}
 
 void UART_CommandInterface::sendReplies(std::vector<CommandResult>& results,CommandInterface* originalInterface){
-	if( (!enableBroadcastFromOtherInterfaces && originalInterface != this) || !tud_ready() ){
+	if( (!enableBroadcastFromOtherInterfaces && originalInterface != this) ){
 		return;
 	}
 
@@ -297,7 +316,7 @@ void UART_CommandInterface::sendReplies(std::vector<CommandResult>& results,Comm
 void UART_CommandInterface::uartRcv(char& buf){
 	uint32_t len = 1;
 	BaseType_t savedInterruptStatus =  cpp_freertos::CriticalSection::EnterFromISR();
-	if(this->parser.bufferCapacity() > len) // Check buffer because we can't allocate more memory inside the ISR safely at the moment
+	if(this->parser.bufferCapacity() > (int32_t)len) // Check buffer because we can't allocate more memory inside the ISR safely at the moment
 		StringCommandInterface::addBuf(&buf, &len);
 	cpp_freertos::CriticalSection::ExitFromISR(savedInterruptStatus);
 }

Firmware/FFBoard/Src/HidCommandInterface.cpp

@@ -52,6 +52,9 @@ void HID_CommandInterface::Run(){
 	}
 }
 
+bool HID_CommandInterface::readyToSend(){
+	return this->outBuffer.size() < maxQueuedReplies;
+}
 
 void HID_CommandInterface::sendReplies(std::vector<CommandResult>& results,CommandInterface* originalInterface){
 
@@ -61,6 +64,10 @@ void HID_CommandInterface::sendReplies(std::vector<CommandResult>& results,Comma
 			continue;
 
 		if( (originalInterface != this && enableBroadcastFromOtherInterfaces) || ( result.type == CommandStatus::OK && replies.empty() ) ){ // Request was sent by a different interface
+			if(this->outBuffer.size() > maxQueuedRepliesBroadcast){
+				continue; // for now we just throw away broadcasts if the buffer contains too many pending replies.
+			}
+
 			CommandReply reply; // Fake reply
 
 			// return original command as get value if it was a set command

Firmware/FFBoard/Src/global_callbacks.cpp

@@ -237,10 +237,13 @@ uint8_t const * tud_hid_descriptor_report_cb(uint8_t itf){
 
 void tud_cdc_rx_cb(uint8_t itf){
 	pulseSysLed();
-	uint8_t buf[64];
-	uint32_t count = tud_cdc_n_read(itf,buf, sizeof(buf));
-	if(mainclass!=nullptr)
-		mainclass->cdcRcv((char*)buf,&count);
+//	uint8_t buf[64];
+	if(mainclass!=nullptr){
+		mainclass->cdcRcvReady(itf);
+	}
+//		uint32_t count = tud_cdc_n_read(itf,buf, std::min<uint32_t>(mainclass->cdcFree(),sizeof(buf)));
+//		mainclass->cdcRcv((char*)buf,&count);
+//	}
 }
 
 void tud_cdc_tx_complete_cb(uint8_t itf){