IceCore myStorm firmware

Folknology 2019-07-31 07:34:29 UTC #121

Actually you have CPPFLAGS and CPFLAGS, the CPPFLAGS were there before we even had any C++, I am not sure why they were named as such, they are actually generic flags for both C and C++. The Flags added for C++ are actually the CPFLAGS which is a little confusing I know. I am however not a make guru and love any feedback to improve the makefile..

Folknology 2019-07-31 07:42:48 UTC #122

@lawrie can you explain to me what is happening in the Flash_Xfer :

uint8_t flash_xfer(uint8_t data) {
  write_u32(data | 0x01000000, SPI_FLASH_XFER);

  for(int i=0;i<1000;i++) {
    uint32_t r = read_u32(SPI_FLASH_XFER);
    if ((r & SPI_FLASH_RX_VALID)) return r & 0xff;
  }
  return 0xFF;
}

In particular the for loop, is this a delay, does the read pick up data if so what data, looks like an MSb check?

zub 2019-07-31 07:52:02 UTC #123

Typically, flags for C++ compiler are named CXXFLAGS. "CPP" is used for the C (and C++) preprocessor, though one would rarely run that separately. See e.g. https://stackoverflow.com/questions/495598/difference-between-cppflags-and-cxxflags-in-gnu-make

AndyCap 2019-07-31 08:07:18 UTC #124

Thanks guys, that was confusing me!

lawrie 2019-07-31 08:10:45 UTC #125

I think that is specific to the SpinalHDL SPI library memory mapping. The SPI xfer register has a top bit indicating that the data is valid and when it is, the least-significant byte is the data. Or, in fact, the library can deal with data of greater widths when needed. So the data is the 8-bits shifted in from the MISO pin.

The whole xfer function is just the equivalent of HAL_SPI_TransmitReceive of one byte.

Folknology 2019-07-31 08:24:30 UTC #126

OK I am working on adding the Flash functions to enable read/writing and Erase this morning hence the query. These low level methods will enable us to program the flash on the STM32

lawrie 2019-07-31 08:26:19 UTC #127

There was a typo there, I meant:

The whole xfer function is just the equivalent of HAL_SPI_TransmitReceive of one byte.

A lot of the calls to flash_xfer, which just write data, can be replaced with HAL_SPI_Transmit and calls to flash_xfer(0) which read the return value to HAL_SPI_Receive.

AndyCap 2019-07-31 08:27:59 UTC #128

Hi Guys,

I have implemented a way of handling different commands, ICE40 program, flash program etc which is backward compatible with the existing format.

There is a new class CommandStream:

class CommandStream
{
public:
	CommandStream(void)
	{
		Init();
	};

	void Init(void);
	void AddCommandHandler(uint8_t uCommandByte, CommandHandler *pCommandHandler);

	uint8_t stream(uint8_t *data, uint32_t len);

private:
	typedef enum { stDetect, stDetectCommandByte, stDispatch, stProcessing, stError } StreamState;

	StreamState m_state;

	uint8_t			m_header[3] = {0x7e, 0xaa, 0x99};
	uint8_t			m_uHeaderScanPos = 0;

	CommandHandler		*m_pCurrentCommandHandler;
	CommandHandler 		*m_commandHandlers[256] = {0};
};

This is used by the USB callback instead of the Ice40 object:

CommandStream 	g_commandStream;

static int8_t usbcdc_rxcallback(uint8_t *data, uint32_t *len){

	USBD_CDC_SetRxBuffer(&hUsbDeviceFS, &data[0]);

#define COMMAND_HANDLER

#ifdef COMMAND_HANDLER
	if(*len)
	{
		if(!g_commandStream.stream(data, *len))
		{
			// HAL_UART_Transmit(&huart1, data, *len, HAL_UART_TIMEOUT_VALUE);
			// mode_led_toggle();

			HAL_UART_Transmit_DMA(&huart1, data, *len);
			return USBD_OK;
			//if(temp) mode_led_low();
		}
	}
#else
	if(*len)
		if(!Ice40.stream(data, *len)){
			// HAL_UART_Transmit(&huart1, data, *len, HAL_UART_TIMEOUT_VALUE);
			// mode_led_toggle();

			HAL_UART_Transmit_DMA(&huart1, data, *len);
			return USBD_OK;
			//if(temp) mode_led_low();
		}
#endif
	USBD_CDC_ReceivePacket(&hUsbDeviceFS);

	return USBD_OK;
}

There is a pure virtual class CommandHandler that any command needs to inherit from:

class CommandHandler
{
public:
	virtual bool streamData(uint8_t *data, uint32_t len, bool bEndStream) = 0;
	virtual bool init(void) = 0;
};

init() is called when the Command is recognised in the stream.
streamData() is called with the data from the stream, the 4 byte header 0x7E, 0xAA, 0x99, 0xNN is removed

in Setup() we register our CommandHandlers against a command byte, this is the last byte of the 4 byte header 0x7E, 0xAA, 0x99, 0xNN:

	g_commandStream.AddCommandHandler(0x7e, &Ice40);
	g_commandStream.AddCommandHandler(0x01, &g_flash);
	g_commandStream.AddCommandHandler(0x02, &g_count);

So for the FPGA programming Fpga now inherits from CommandHandler and implements:

bool Fpga::init(void)
{
	bool bResult = false;
	nbytes = 0;
	status_led_high();
	flash_SPI_Disable();
	if (err = reset(MCNTRL))
		flash_SPI_Enable();
	else
	{
		// Send header as this is removed from the stream
		nbytes+=4;
		write((uint8_t *)&sig, 4);
		bResult = true;
	}

	return bResult;
}

bool Fpga::streamData(uint8_t *data, uint32_t len, bool bEndStream){
	bool bResult = true;

	nbytes += len;
	write(data, len);

	if(nbytes >= NBYTES)
	{
		if(err = config())
			status_led_high();
		else
			status_led_low();
		flash_SPI_Enable();
		bResult = false;
	}

	return bResult;
}

I have added a test command that counts the bytes received:

class Count : public CommandHandler
{
public:
	virtual bool streamData(uint8_t *data, uint32_t len, bool bEndStream);
	virtual bool init(void);

private:
	uint32_t m_uCount;
};

bool Count::init(void)
{
	cdc_puts("Count::init()\n");
	m_uCount = 0;
	return true;
}

bool Count::streamData(uint8_t *data, uint32_t len, bool bEndStream)
{
	m_uCount+= len;
	if(bEndStream)
	{
		char buffer[128];
		sprintf(buffer, "%lu bytes received\n", m_uCount);
		cdc_puts(buffer);
		return false;
	}
	else
		return true;
}

so the following sent:

Mac-Pro:trail andrewcapon$ cat count.hex
ff 00 00 ff 7e aa 99 02
Mac-Pro:trail andrewcapon$ xxd -r -p count.hex count.bin
Mac-Pro:trail andrewcapon$ cat count.bin chip.bin > countchip.bin
Mac-Pro:trail andrewcapon$ sudo cat countchip.bin  > /dev/cu.usbmodem00000000001A1

Gets back from uart

Count::init()
135100 bytes received

and sudo cat chip.bin > /dev/cu.usbmodem00000000001A1 still programs the ICE40 as before.

Also CommandStream handles headers that are byte aligned and can straddle usb packets.

It all works, is it a solution you would be interested in?

lawrie 2019-07-31 09:02:02 UTC #129

Their are extra flash commands you can use. For example, the TinyFPGA BX uses a command (0x20) that erases just 4096 bytes:

github.com

lawrie/TinyFPGA-Bootloader/blob/master/programmer/tinyprog/init.py#L293


def write_enable(self):
    self.cmd(0x06)


def write_disable(self):
    self.cmd(0x04)


def wait_while_busy(self):
    while to_int(self.read_sts()) & 1:
        pass


def _erase(self, addr, length):
    opcode = {
         4 * 1024: 0x20,
        32 * 1024: 0x52,
        64 * 1024: 0xd8,
    }[length]
    self.write_enable()
    self.cmd(opcode, addr)
    self.wait_while_busy()


def erase(self, addr, length, disable_progress=True):

Folknology 2019-07-31 09:02:15 UTC #130

@AndyCap This is very interesting and a good abstraction making it better for folks implementing extra command handlers, there is a lot I wouuld like to do for both IceCore and other cores coming down later that could use extended functionalities.

One area I lack enough knowledge/experience of is the dynamic dispatch in C++ by using virtual functions in your command abstraction is that adding overhead. Remember in the callback we are effectively in an interrupt and need to process quickly, function calls in interrupts add overhead but can often be optimised using inlining etc.. Any one here understand the mechanics of the virtual functions and what that means in terms of overheads?

Folknology 2019-07-31 09:03:44 UTC #131

That's interesting and useful for optimisation, I am trying to keep things simple to start with

AndyCap 2019-07-31 09:30:10 UTC #132

There is one level of indirection (lookup in vtable) and a method call. The compiler will most likely not inline the virtual call is it is done from a pointer.

I would say compared to the rest of the code going on it is negligible, though. I can so some timings...

AndyCap 2019-07-31 10:06:24 UTC #133

Ok, I have done some timings.

I added an inlined version of stream into the FPGA and Count classes.

I compiled with -03

I tested with the following code:

	// timings
	uint8_t buffer[64];
	CommandHandler *pFpgaHandler = &Ice40;
	CommandHandler *pCountHandler = &g_count;

	// Fpga commandHandler
	uint32_t uStartTicks = HAL_GetTick();
	for(int i=0; i < 1000; i++)
		Ice40.streamDataInlined(buffer, 64, false);

	uint32_t uInlineFpgaTicks = HAL_GetTick();

	for(int i=0; i < 1000; i++)
		pFpgaHandler->streamData(buffer, 64, false);

	uint32_t uVirtualFpgaTicks = HAL_GetTick();


	// Count command handler
	for(int i=0; i < 1000; i++)
			g_count.streamDataInlined(buffer, 64, false);

	uint32_t uInlineCountTicks = HAL_GetTick();

	for(int i=0; i < 1000; i++)
		pCountHandler->streamData(buffer, 64, false);

	uint32_t uVirtualCountTicks = HAL_GetTick();


	uInlineFpgaTime = uInlineFpgaTicks-uStartTicks;
	uVirtuaFpgalTime = uVirtualFpgaTicks-uInlineFpgaTicks;

	uInlineCountTime = uInlineCountTicks - uVirtualFpgaTicks;
	uVirtualCountTime = uVirtualCountTicks - uInlineCountTicks;

and we get:

	uInlineFpgaTime = 450
	uVirtuaFpgalTime = 450

	uInlineCountTime = 0
	uVirtualCountTime = 0

I'm sure if we had a higher resolution timer we would see some difference though, do you know of one in the STM World?

Folknology 2019-07-31 10:25:10 UTC #134

I tend to toggle GPIO pins and measure time on the scope, but that's not always possible if you don't have one

Folknology 2019-07-31 10:36:42 UTC #135

Looking underneath at the HAL I think it might be low-res tick by default :
/** * @brief This function is called to increment a global variable "uwTick" * used as application time base. * @note In the default implementation, this variable is incremented each 1ms * in SysTick ISR. * @note This function is declared as __weak to be overwritten in case of other * implementations in user file. * @retval None */ __weak void HAL_IncTick(void) { uwTick += uwTickFreq; }

AndyCap 2019-07-31 10:37:45 UTC #136

Yeah, I think it is ms based.

AndyCap 2019-07-31 10:38:18 UTC #137

Still basically there is really no overhead of the virtual calls compared to inlined ones (for our purposes)

Folknology 2019-07-31 10:40:45 UTC #138

You might be able to query timer 6 however be careful as I think it maybe only 16 bit and will prob reset every few milliseconds

Folknology 2019-07-31 10:50:33 UTC #139

Well I would still like to see the actual overhead, but you have a point in the case of transferring a full FS USB packet. However, given that our dispatch is based on that 4th byte a switch at runtime (given we know all the installed commands) might be more effective than a dynamic dispatch? Sorry if I am being anal here it's a difficult habit to give up for a barebones µC coder

BTW I have not optimised things like FPGA::Write yet either, we may be able to get that running a lot faster..

zub 2019-07-31 10:55:10 UTC #140

I think the result can be similar to this in (slightly invalid) C:

typedef struct
{
	bool (*streamData)(CommandHandler *this, uint8_t *data, uint32_t len, bool bEndStream);
	bool (*init)(CommandHandler *this);
} CommandHandlerVTBL;

bool CommandHandlerA_streamData(CommandHandlerA *this, vuint8_t *data, uint32_t len, bool bEndStream)
{
	...
}

bool CommandHandlerA_init(CommandHandlerA *this)
{
	...
}

CommandHandlerVTBL CommandHandlerA_VTBL =
{
	&CommandHandlerA_streamData,
	&CommandHandlerA_init
};

typedef struct CommandHandler
{
	CommandHandlerVTBL * vtbl;
	...
}

typedef CommandHandlerA
{
	CommandHandlerClass base;
	...
}

void foo()
{
	// CommandHandlerA a;
	CommandHandlerA a;
	a.base.vtbl = &CommandHandlerA_VTBL;

	// a.init();
	a.base.vtbl->init(&a);
}

The compiler in general can't inline virtual calls, though sometimes it is possible. Using final keyword may help here. And maybe building with with -flto. But there are also other possible approaches in C++ how to avoid virtual calls, like static polymorphism and template magic.