Skip to content
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

Facilitate support for FlexiHAL board in WebBuilder #217

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

engigeer
Copy link

Below is a summary of the changes included in this PR to facilitate support for FlexiHAL board in webbuilder. It was my intention to include the absolute minimum of changes necessary to compile a feature-complete binary without errors, but would welcome any suggestions if something appears to be extraneous or represents a reversion. I have used code guards, where it seemed appropriate, to hopefully reduce the risk of introducing unintentional bugs for other boards, but I do also hope that some changes may provide benefits to all upstream users:

.uf2 support (improvement)

  • merge changes main.c (clock reset prior to grbl_enter) from Expatria fork + add code guard
  • add .py and .json files for .uf2 conversion
  • add post-build script to generate .uf2 binary
  • add $UF2 command to driver.c to provide entry point to bootloader (inspired by existing $DFU command)

i2c fastmode (bugfix)

  • merge changes to i2c.c and from Expatria fork

USB timeout (bugfix)

  • merge changes to usb_serial.c from Expatria fork

shared SPI for SD card + Wiznet (bugfix)

  • merge changes to diskio.c from Expatria fork
  • merge changes to driver.c from Expatria fork
  • merge changes to spi.c from Expatria fork + add code guard

other changes / additions

  • add flexiHAL linker script
  • merge updates to flexi_hal_map.h and flexi_hal.c from Expatria fork
  • update platformio.ini and add missing build flags

Copy link
Contributor

@terjeio terjeio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs a bit more work, some code should me moved and some even removed?

HAL_FMPI2CEx_ConfigAnalogFilter(&i2c_port, FMPI2C_ANALOGFILTER_ENABLE);
HAL_FMPI2C_Init(&i2c_port);
__HAL_FMPI2C_ENABLE(&i2c_port);
HAL_NVIC_SetPriority(FMPI2C1_EV_IRQn, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to have the I2C priority at the same level as step generation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this may be undesirable, although it also seems that UART and SPI DMA may use this same highest priority (presumably without issue). Do you have any suggestion on what would be a preferred priority for i2c?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try 0, 2 - I2C comms is handshaked so that should be ok. BTW I want to upgrade I2C comms to use DMA...
And why the extra call to __HAL_FMPI2C_ENABLE(&i2c_port);? Is the one in HAL_FMPI2C_Init() misplaced/not working?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having some issues getting the i2c comms to work and ended up adding the enable along with the init, but I now see it is redundant. I will remove.

@@ -278,11 +280,17 @@ nvs_transfer_result_t i2c_nvs_transfer (nvs_transfer_t *i2c, bool read)

// while (HAL_I2C_IsDeviceReady(&i2c_port, (uint16_t)(0xA0), 3, 100) != HAL_OK);
HAL_StatusTypeDef ret;

#ifdef I2C_FASTMODE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

@@ -169,7 +169,44 @@ void spi_init (void)
if(!init) {

#if SPI_PORT == 1
#ifdef BOARD_FLEXI_HAL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only allow board specific code in a very few places, a new SPI_PORT 1x number should be added instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I see now how this should be done. I will adjust to use SPI_PORT 13 which seems to be the next available.

@@ -78,18 +78,26 @@ static void usbRxCancel (void)
static inline bool usb_write (void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? Workaround for USB blocking keypad when not connected? If so this is fixed by not outputting to USB if the DTR line is not asserted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I overlooked that this had been patched elsewhere. I will remove.

* @brief This function handles DMA2 stream3 global interrupt.
*/

static void MX_DMA_Init (void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DMA code should be moved to spi.c?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe if I define SPI_PORT 13 as you suggest above this is redundant and I will remove.

@@ -139,7 +139,7 @@ BYTE wait_ready (void)
//static
void send_initial_clock_train(void)
{
unsigned int i = 10;
unsigned int i = 50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change may be unnecessary. I will test the sd card functionality with it reverted back to 10 and confirm.


status_code_t enter_uf2 (sys_state_t state, char *args)
{
extern uint32_t _board_dfu_dbl_tap; /* Symbol defined in the linker script */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if not?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I suspect it may throw a compilation error. I will test and try to refactor to ensure this is not the case.

NVIC_SystemReset();

return Status_OK;
}

const sys_command_t boot_command_list[2] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move back to the original place. And UF2 has to be included only when available? By adding a new symbol for it?
If board specific the UF2 command should be added to flexi_hal.c instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I received compilation errors when the boot command list had a size > 1 in the original location, but I think this should not be an issue if I separate dfu and uf2 as you suggest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, and do not specify the size of the array - the compiler figures that out.

Comment on lines +98 to +120
#if defined(BOARD_FLEXI_HAL)
RCC_OscInitTypeDef RCC_OscInitStruct = {0};
__HAL_PWR_VOLTAGESCALING_CONFIG(PWR_REGULATOR_VOLTAGE_SCALE1);

RCC_OscInitStruct.OscillatorType = RCC_OSCILLATORTYPE_HSE;
RCC_OscInitStruct.HSEState = RCC_HSE_ON;
RCC_OscInitStruct.PLL.PLLState = RCC_PLL_ON;
RCC_OscInitStruct.PLL.PLLSource = RCC_PLLSOURCE_HSE;
RCC_OscInitStruct.PLL.PLLM = 15;
RCC_OscInitStruct.PLL.PLLN = 216;
RCC_OscInitStruct.PLL.PLLP = RCC_PLLP_DIV2;
RCC_OscInitStruct.PLL.PLLQ = 8;
RCC_OscInitStruct.PLL.PLLR = 2;

#define APB1CLKDIV RCC_HCLK_DIV4
#define APB2CLKDIV RCC_HCLK_DIV2

if (HAL_PWREx_EnableOverDrive() != HAL_OK)
{
Error_Handler();
}

#define FLASH_LATENCY FLASH_LATENCY_5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +446 to +449
#if defined(BOARD_FLEXI_HAL)
__HAL_RCC_USB_OTG_FS_CLK_ENABLE();
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why this for FlexiHAL only? It is called from USBD_LL_Init() during USB initialisation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants