Skip to content

Add spi_get_peripheral_name() to stm_spi #9845

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Add spi_get_peripheral_name() to stm_spi #9845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_NUCLEO_F429ZI/PeripheralNames.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ typedef enum {
UART_8 = (int)UART8_BASE
} UARTName;

#define SPI_COUNT 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this addition is not needed for this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't until yesterday.
#9469 introduced the static array (it was put back yesterday), so it is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#9469 is not merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not, but as said in the description, this PR depends on #9469

Copy link
Contributor

Choose a reason for hiding this comment

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

Then all the STM targets need to be updated, why updating F429ZI only ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Background is stated on the description. Other STM targets would work like they would before like stated in the #9469

No, it means that targets that don't provide this extra facility retain the existing single whole-system SPI mutex.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why would this change be valid for WISUN and F429 only ? Does the change have drawbacks that we don't want to deploy everywhere ? If there are only advantages and no drawbacks, I'd prefer to have the change deployed widely. We try to keep all STM targets behavior as aligned as possible otherwise maintenance becomes more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK there are mostly advantages, bit larger memory footprint because of the static array for peripherals is a drawback though. The changes done now are preceding a major change to SPI that are coming in a future release. (That will effectively overwrite the changes done now)

Wi-SUN testing has been mainly done with F429, so that is why it is the first one to get changes. Changes to different HAL's can be added later on if there is a need.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then, but when the "major changes" are made, all targets should be aligned.

typedef enum {
SPI_1 = (int)SPI1_BASE,
SPI_2 = (int)SPI2_BASE,
Expand Down
18 changes: 18 additions & 0 deletions targets/TARGET_STM/stm_spi_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,24 @@ void init_spi(spi_t *obj)
}
}

SPIName spi_get_peripheral_name(PinName mosi, PinName miso, PinName sclk) {
SPIName spi_mosi = (SPIName)pinmap_peripheral(mosi, PinMap_SPI_MOSI);
SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of "3 wires SPI" , miso is set as "NC" - so this needs to be taken into account.
Please add check, if miso is NC, only merge mosi and sclk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pinmap_merge should handle a situation when other pin is set as NC, but I'll provide also a check here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs more change. The check should happen before calling to
SPIName spi_miso = (SPIName)pinmap_peripheral(miso, PinMap_SPI_MISO);

Otherwise, the system would crash here:
https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L78

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If miso is NC, should it even get there?
https://github.com/ARMmbed/mbed-os/blob/master/hal/mbed_pinmap_common.c#L73

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right - so that's OK - thanks !

SPIName spi_sclk = (SPIName)pinmap_peripheral(sclk, PinMap_SPI_SCLK);

SPIName spi_per;

// If 3 wire SPI is used, the miso is not connected.
if (miso == NC) {
spi_per = (SPIName)pinmap_merge(spi_mosi, spi_sclk);
} else {
SPIName spi_data = (SPIName)pinmap_merge(spi_mosi, spi_miso);
spi_per = (SPIName)pinmap_merge(spi_data, spi_sclk);
}

return spi_per;
}

void spi_init(spi_t *obj, PinName mosi, PinName miso, PinName sclk, PinName ssel)
{
struct spi_s *spiobj = SPI_S(obj);
Expand Down