Skip to content

Add explicit pin-map support for extra targets #11632

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 20 commits into from
Oct 29, 2019
Merged

Add explicit pin-map support for extra targets #11632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
171f9bb
Add qspi_init_direct() function to HAL API
mprse Sep 23, 2019
8fd0184
Add can_init_direct() functions to HAL API
mprse Sep 23, 2019
993843b
CAN: Add explicit pinmap support
mprse Sep 23, 2019
61ba60b
QSPI: Add explicit pinmap support
mprse Sep 23, 2019
6e6d597
Explicit pinmap: Add support for CAN and QSPI
mprse Sep 23, 2019
d3e6b3c
STM CAN driver: Add explicit pinmap support
mprse Sep 23, 2019
8dc1172
STM QSPI driver: Add explicit pinmap support
mprse Oct 1, 2019
9a9c329
DISCO_L475VG_IOT01A: Add explicit pinmap support
mprse Sep 23, 2019
82eb7aa
NUCLEO_F411RE: Add explicit pinmap support
mprse Sep 20, 2019
0f19604
NUCLEO_L073RZ: Add explicit pinmap support
mprse Sep 27, 2019
0642808
LPC55S69_NS: Add explicit pinmap support
mprse Sep 30, 2019
9b7b4a1
NUCLEO_F303RE: Add explicit pinmap support
mprse Sep 30, 2019
8c10728
NRF52840_DK: Add explicit pinmap support
mprse Sep 30, 2019
7e87cf0
STM: Add support for internal ADC pins
mprse Sep 30, 2019
82aefe7
NUCLEO_F429ZI add CAN pinmaps
mprse Oct 2, 2019
21a9712
QSPI: Add pointer to HAL init function
mprse Oct 24, 2019
e048619
get_qspi_pinmap(): Fix invalid condition
mprse Oct 24, 2019
ba95fcf
CAN: remove redundant loop
mprse Oct 24, 2019
9813021
prevent passing of temporary pinmap objects
mprse Oct 24, 2019
d38c7d6
Serial, QSPI: Fix style
mprse Oct 25, 2019
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 drivers/AnalogIn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class AnalogIn {
* @param pinmap reference to structure which holds static pinmap.
*/
AnalogIn(const PinMap &pinmap);
AnalogIn(const PinMap &&) = delete; // prevent passing of temporary objects

/** Create an AnalogIn, connected to the specified pin
*
Expand Down
1 change: 1 addition & 0 deletions drivers/AnalogOut.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class AnalogOut {
*
* @param pinmap reference to structure which holds static pinmap.
*/
AnalogOut(const PinMap &&) = delete; // prevent passing of temporary objects
AnalogOut(const PinMap &pinmap)
{
analogout_init_direct(&_dac, &pinmap);
Expand Down
19 changes: 18 additions & 1 deletion drivers/CAN.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,24 @@ class CAN : private NonCopyable<CAN> {
*/
CAN(PinName rd, PinName td, int hz);

/** Initialize CAN interface
*
* @param pinmap reference to structure which holds static pinmap
* @param td the transmit pin
* @param hz the bus frequency in hertz
*/
CAN(const can_pinmap_t &pinmap);
CAN(const can_pinmap_t &&) = delete; // prevent passing of temporary objects

/** Initialize CAN interface and set the frequency
*
* @param pinmap reference to structure which holds static pinmap
* @param td the transmit pin
* @param hz the bus frequency in hertz
*/
CAN(const can_pinmap_t &pinmap, int hz);
CAN(const can_pinmap_t &&, int) = delete; // prevent passing of temporary objects

virtual ~CAN();

/** Set the frequency of the CAN interface
Expand Down Expand Up @@ -343,4 +361,3 @@ class CAN : private NonCopyable<CAN> {
#endif

#endif // MBED_CAN_H

1 change: 1 addition & 0 deletions drivers/I2C.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ class I2C : private NonCopyable<I2C> {
* @param explicit_pinmap reference to structure which holds static pinmap.
*/
I2C(const i2c_pinmap_t &explicit_pinmap);
I2C(const i2c_pinmap_t &&) = delete; // prevent passing of temporary objects

/** Set the frequency of the I2C interface
*
Expand Down
1 change: 1 addition & 0 deletions drivers/I2CSlave.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class I2CSlave {
* @param explicit_pinmap reference to structure which holds static pinmap.
*/
I2CSlave(const i2c_pinmap_t &explicit_pinmap);
I2CSlave(const i2c_pinmap_t &&) = delete; // prevent passing of temporary objects

/** Set the frequency of the I2C interface.
*
Expand Down
1 change: 1 addition & 0 deletions drivers/PwmOut.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class PwmOut {
* @param pinmap reference to structure which holds static pinmap.
*/
PwmOut(const PinMap &pinmap);
PwmOut(const PinMap &&) = delete; // prevent passing of temporary objects

~PwmOut();

Expand Down
16 changes: 16 additions & 0 deletions drivers/QSPI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,19 @@ class QSPI : private NonCopyable<QSPI> {
*
*/
QSPI(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, PinName ssel = NC, int mode = 0);

/** Create a QSPI master connected to the specified pins
*
* io0-io3 is used to specify the Pins used for Quad SPI mode
*
* @param pinmap reference to structure which holds static pinmap
* @param mode Clock polarity and phase mode (0 - 3) of SPI
* (Default: Mode=0 uses CPOL=0, CPHA=0, Mode=1 uses CPOL=1, CPHA=1)
*
*/
QSPI(const qspi_pinmap_t &pinmap, int mode = 0);
QSPI(const qspi_pinmap_t &&, int = 0) = delete; // prevent passing of temporary objects

virtual ~QSPI()
{
}
Expand Down Expand Up @@ -222,13 +235,16 @@ class QSPI : private NonCopyable<QSPI> {
int _mode; //SPI mode
bool _initialized;
PinName _qspi_io0, _qspi_io1, _qspi_io2, _qspi_io3, _qspi_clk, _qspi_cs; //IO lines, clock and chip select
const qspi_pinmap_t *_explicit_pinmap;
Copy link
Contributor

@kjbracey kjbracey Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Are you requiring the passed-in pinmap object to have lifetime extending beyond the constructor call? That's not specified.

Looking at the code, I don't think you are, so this doesn't have to be a member. You could just pass the pinmap to the _initialize_direct. Will save 4 bytes of RAM. (Nope - you should be calling _initialize_direct other places)

I realise this model comes from the SPI code, which does require the pinmap to persist for later re-init. That one does need that specified. Maybe the same should be specified for all classes to allow suspend/resume in future, even if they don't do it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pondering the persistence thing - this means that

SPI(get_spi_pinmap(A,B,C,D))

which I've seen suggested is actually illegal. That would be retaining a pointer to a temporary.

It's unfortunate that the compiler accepts it. You're allowed to pass a temporary X to a const X &

Two suggestions to prevent that here: https://stackoverflow.com/questions/40790126/prevent-passing-temporary-for-const-ref-parameter

  1. Delete the overload with r-value parameter: SPI(const spi_pinmap_t &&) = delete
  2. Use const pointer: SPI(const spi_pinmap_t *)

Not sure it's too worth worrying about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mentioned form SPI(get_spi_pinmap(A,B,C,D)) is actually not suggested. According to the design doc:

It is strongly suggested that the result of get_xxx_pinmap() calls should normally be used in explicitly constant expression contexts like initializing a constexpr variable, to guarantee that the tables are not pulled in.

I think we should prevent passing of temporary objects. The first option looks more suitable for our case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added
SPI(const spi_pinmap_t &&, PinName) = delete;

And now when SPI spi(get_spi_pinmap(PA_7, PA_6, PA_5, PA_4)); causes compilation error:

[Error] main.cpp@41,9: call to deleted constructor of 'mbed::SPI'
[ERROR] .\main.cpp:41:9: error: call to deleted constructor of 'mbed::SPI'
    SPI spi(get_spi_pinmap(PA_7, PA_6, PA_5, PA_4));
        ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./drivers/SPI.h:146:5: note: 'SPI' has been explicitly marked deleted here
    SPI(const spi_pinmap_t &&) = delete;
    ^
1 error generated.

This is what we wanted to achieve. The last question is if this solution should be added only to SPI and QSPI classes or to all peripheral constructors (which support explicit pinmap)?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the minute, the others do support passing a temporary, I guess, because they copy all the data out in the constructor.

Removing that ability to pass a temporary later (eg if they wanted to implement suspend/resume) would be an API change, so I guess it's probably better to ensure it's blocked now. And it prevents the bad code generation possibility.

bool (QSPI::* _init_func)(void);

private:
/* Private acquire function without locking/unlocking
* Implemented in order to avoid duplicate locking and boost performance
*/
bool _acquire(void);
bool _initialize();
bool _initialize_direct();

/*
* This function builds the qspi command struct to be send to Hal
Expand Down
2 changes: 2 additions & 0 deletions drivers/SPI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ class SPI : private NonCopyable<SPI> {
* @param explicit_pinmap reference to structure which holds static pinmap.
*/
SPI(const spi_pinmap_t &explicit_pinmap);
SPI(const spi_pinmap_t &&) = delete; // prevent passing of temporary objects

/** Create a SPI master connected to the specified pins.
*
Expand All @@ -157,6 +158,7 @@ class SPI : private NonCopyable<SPI> {
* @param ssel SPI Chip Select pin.
*/
SPI(const spi_pinmap_t &explicit_pinmap, PinName ssel);
SPI(const spi_pinmap_t &&, PinName) = delete; // prevent passing of temporary objects

virtual ~SPI();

Expand Down
1 change: 1 addition & 0 deletions drivers/SPISlave.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class SPISlave : private NonCopyable<SPISlave> {
* @param explicit_pinmap reference to structure which holds static pinmap.
*/
SPISlave(const spi_pinmap_t &pinmap);
SPISlave(const spi_pinmap_t &&) = delete; // prevent passing of temporary objects

/** Configure the data transmission format.
*
Expand Down
2 changes: 2 additions & 0 deletions drivers/Serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class Serial : public SerialBase, public Stream, private NonCopyable<Serial> {
* Either tx or rx may be specified as NC (Not Connected) if unused
*/
Serial(const serial_pinmap_t &explicit_pinmap, const char *name = NULL, int baud = MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE);
Serial(const serial_pinmap_t &&, const char * = NULL, int = MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE) = delete; // prevent passing of temporary objects

/** Create a Serial port, connected to the specified transmit and receive pins, with the specified baud
*
Expand All @@ -104,6 +105,7 @@ class Serial : public SerialBase, public Stream, private NonCopyable<Serial> {
* Either tx or rx may be specified as NC (Not Connected) if unused
*/
Serial(const serial_pinmap_t &explicit_pinmap, int baud);
Serial(const serial_pinmap_t &&, int) = delete; // prevent passing of temporary objects

/* Stream gives us a FileHandle with non-functional poll()/readable()/writable. Pass through
* the calls from the SerialBase instead for backwards compatibility. This problem is
Expand Down
22 changes: 13 additions & 9 deletions drivers/source/CAN.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,28 @@ namespace mbed {
CAN::CAN(PinName rd, PinName td) : _can(), _irq()
{
// No lock needed in constructor

for (size_t i = 0; i < sizeof _irq / sizeof _irq[0]; i++) {
_irq[i] = NULL;
}

can_init(&_can, rd, td);
can_irq_init(&_can, (&CAN::_irq_handler), (uint32_t)this);
}

CAN::CAN(PinName rd, PinName td, int hz) : _can(), _irq()
{
// No lock needed in constructor
can_init_freq(&_can, rd, td, hz);
can_irq_init(&_can, (&CAN::_irq_handler), (uint32_t)this);
}

for (size_t i = 0; i < sizeof _irq / sizeof _irq[0]; i++) {
_irq[i] = NULL;
}
CAN::CAN(const can_pinmap_t &pinmap) : _can(), _irq()
{
// No lock needed in constructor
can_init_direct(&_can, &pinmap);
can_irq_init(&_can, (&CAN::_irq_handler), (uint32_t)this);
}

can_init_freq(&_can, rd, td, hz);
CAN::CAN(const can_pinmap_t &pinmap, int hz) : _can(), _irq()
{
// No lock needed in constructor
can_init_freq_direct(&_can, &pinmap, hz);
can_irq_init(&_can, (&CAN::_irq_handler), (uint32_t)this);
}

Expand Down
50 changes: 48 additions & 2 deletions drivers/source/QSPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ QSPI::QSPI(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, Pin
_qspi_io3 = io3;
_qspi_clk = sclk;
_qspi_cs = ssel;
_explicit_pinmap = NULL;
_inst_width = QSPI_CFG_BUS_SINGLE;
_address_width = QSPI_CFG_BUS_SINGLE;
_address_size = QSPI_CFG_ADDR_SIZE_24;
Expand All @@ -59,9 +60,36 @@ QSPI::QSPI(PinName io0, PinName io1, PinName io2, PinName io3, PinName sclk, Pin
_mode = mode;
_hz = ONE_MHZ;
_initialized = false;
_init_func = &QSPI::_initialize;

//Go ahead init the device here with the default config
bool success = _initialize();
bool success = (this->*_init_func)();
MBED_ASSERT(success);
}

QSPI::QSPI(const qspi_pinmap_t &pinmap, int mode) : _qspi()
{
_qspi_io0 = pinmap.data0_pin;
_qspi_io1 = pinmap.data1_pin;
_qspi_io2 = pinmap.data2_pin;
_qspi_io3 = pinmap.data3_pin;
_qspi_clk = pinmap.sclk_pin;
_qspi_cs = pinmap.ssel_pin;
_explicit_pinmap = &pinmap;
_inst_width = QSPI_CFG_BUS_SINGLE;
_address_width = QSPI_CFG_BUS_SINGLE;
_address_size = QSPI_CFG_ADDR_SIZE_24;
_alt_width = QSPI_CFG_BUS_SINGLE;
_alt_size = QSPI_CFG_ALT_SIZE_8;
_data_width = QSPI_CFG_BUS_SINGLE;
_num_dummy_cycles = 0;
_mode = mode;
_hz = ONE_MHZ;
_initialized = false;
_init_func = &QSPI::_initialize_direct;

//Go ahead init the device here with the default config
bool success = (this->*_init_func)();
MBED_ASSERT(success);
}

Expand Down Expand Up @@ -255,12 +283,30 @@ bool QSPI::_initialize()
return _initialized;
}

// Note: Private helper function to initialize qspi HAL
bool QSPI::_initialize_direct()
Copy link
Contributor

Choose a reason for hiding this comment

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

_acquire is still just calling _initialize - don't you need to do the _init_func thing that SPI had?

Copy link
Contributor Author

@mprse mprse Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

QSPI constructor calls HAL init() function, but I missed that _acquire function also calls init(). You are right that the same mechanism as in SPI must be also used here.

{
if (_mode != 0 && _mode != 1) {
_initialized = false;
return _initialized;
}

qspi_status_t ret = qspi_init_direct(&_qspi, _explicit_pinmap, _hz, _mode);
if (QSPI_STATUS_OK == ret) {
_initialized = true;
} else {
_initialized = false;
}

return _initialized;
}

// Note: Private function with no locking
bool QSPI::_acquire()
{
if (_owner != this) {
//This will set freq as well
_initialize();
(this->*_init_func)();
_owner = this;
}

Expand Down
10 changes: 10 additions & 0 deletions hal/can_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,22 @@ typedef enum {
MODE_TEST_SILENT
} CanMode;

typedef struct {
int peripheral;
PinName rd_pin;
int rd_function;
PinName td_pin;
int td_function;
} can_pinmap_t;

typedef void (*can_irq_handler)(uint32_t id, CanIrqType type);

typedef struct can_s can_t;

void can_init(can_t *obj, PinName rd, PinName td);
void can_init_direct(can_t *obj, const can_pinmap_t *pinmap);
void can_init_freq(can_t *obj, PinName rd, PinName td, int hz);
void can_init_freq_direct(can_t *obj, const can_pinmap_t *pinmap, int hz);
void can_free(can_t *obj);
int can_frequency(can_t *obj, int hz);

Expand Down
24 changes: 21 additions & 3 deletions hal/explicit_pinmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ MBED_WEAK void pwmout_init_direct(pwmout_t *obj, const PinMap *pinmap)
#if DEVICE_ANALOGIN
MBED_WEAK void analogin_init_direct(analogin_t *obj, const PinMap *pinmap)
{
printf("Pin: %d \r\n", pinmap->pin);
//wait_ns(5000);

analogin_init(obj, pinmap->pin);
}
#endif
Expand Down Expand Up @@ -75,4 +72,25 @@ MBED_WEAK void serial_set_flow_control_direct(serial_t *obj, FlowControl type, c
serial_set_flow_control(obj, type, pinmap->rx_flow_pin, pinmap->tx_flow_pin);
}
#endif

#if DEVICE_CAN
MBED_WEAK void can_init_freq_direct(can_t *obj, const can_pinmap_t *pinmap, int hz)
{
can_init_freq(obj, pinmap->rd_pin, pinmap->td_pin, hz);
}

MBED_WEAK void can_init_direct(can_t *obj, const can_pinmap_t *pinmap)
{
can_init(obj, pinmap->rd_pin, pinmap->td_pin);
}

#endif

#if DEVICE_QSPI
MBED_WEAK qspi_status_t qspi_init_direct(qspi_t *obj, const qspi_pinmap_t *pinmap, uint32_t hz, uint8_t mode)
{
return qspi_init(obj, pinmap->data0_pin, pinmap->data1_pin, pinmap->data2_pin, pinmap->data3_pin, pinmap->sclk_pin, pinmap->ssel_pin, hz, mode);
}
#endif

#endif
Loading