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

STM32: Correct device_has_add flags for bluepill_f103c8 target, fixes #7654 #7700

Merged
merged 4 commits into from Sep 18, 2018

Conversation

Projects
None yet
7 participants
@cesarvandevelde
Contributor

cesarvandevelde commented Aug 5, 2018

Description

This is a small patch to fix the device_has flags for the STM32 bluepill_f103c8 target in targets.json. As of yet, not all the appropriate flags have been added, hence why certain features (e.g. CAN port) will not compile. The issue is described in detail in issue #7654. CLA has (just) been signed and emailed.

Note: I use the PlatformIO toolchain to compile mbed code, and it took me some time to figure out why edits to targets.json were not propagating through the toolchain. In case anyone else struggles with this, the solution is as follows: after editing targets.json, PlatformIO's variants folder needs to be regenerated. This can be done by running tox in the folder ~/.platformio/packages/framework-mbed/platformio/.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@0xc0170

0xc0170 approved these changes Aug 6, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 6, 2018

How was this addition tested (from the referenced issue, CAN was tested, the other 3 drivers?)

Thanks for the contribution!

@cesarvandevelde

This comment has been minimized.

Contributor

cesarvandevelde commented Aug 8, 2018

Thanks for your remarks, @0xc0170. To be honest, I did not test the other three flags, as I assumed everything would work due to the similarities between the two targets. However, I dug further into the issue today, and here's the status:

  • CAN peripheral works, verified in my application
  • I tested the flash peripheral via the NVstore library, and verified that it works on the bluepill target. However, I had to make modifications to the PlatformIO build script in order to get it to compile. I'll submit a PR for this to the PlatformIO repository at a later date.
  • Asynchronous serial works, verified via one of the examples.
  • Flow control does not work because the target does not contain a CTS/RTS pinmap. I will investigate this aspect further, but it should be fixable by copy-pasting the pinmap from the nucleo_f103rb target.

@cmonr cmonr requested a review from ARMmbed/team-st-mcd Aug 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

@cesarvandevelde Thanks for the PR!

@ARMmbed/team-st-mcd Would y'all mind taking a look?

@cesarvandevelde

This comment has been minimized.

Contributor

cesarvandevelde commented Aug 14, 2018

Hi @cmonr, further testing revealed two issues with the current pin map of bluepill_f103c8:

  • CTS/RTS pin map is missing.
  • The CAN pins are set to alternate function 1 instead of 10, probably a bug.

The PR is not ready for merging yet. Please give me a few more days to fix these problems...

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Aug 14, 2018

Hi

@cesarvandevelde
Here is the PeripheralPins.c generated by the script mbed-os/tools/targets/STM32_gen_PeripheralPins.py :

Generated_PeripheralPins.zip

I didn't check the result.

@adbridge adbridge added needs: work and removed needs: review labels Aug 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

@cesarvandevelde Sure thing. Thanks for the heads up!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 27, 2018

@cesarvandevelde Still progressing?

@cesarvandevelde cesarvandevelde force-pushed the cesarvandevelde:bluepill-target-fix branch from abda8c5 to f91bba9 Aug 31, 2018

@cesarvandevelde

This comment has been minimized.

Contributor

cesarvandevelde commented Aug 31, 2018

Hi everyone! Sorry to keep you all waiting, but life has been busy and I've only now found the time to work on this bug. Some further testing, as suggested by @0xc0170, revealed a number of issues with the other features enabled by this patch. I've modified the PeripheralPins.h and PinNames.h based on the files provided by @cmonr, incorporating some changes from the NUCLEO_F103RB target. I've tested all the features separately, and everything seems to be in working order. Please see below for an overview of the tests:

CAN

#include <mbed.h>
#include <CAN.h>

CAN        port(PB_8, PB_9);
DigitalOut led(PB_12);
Serial     pc(PA_2, PA_3);

int main() {
  pc.baud(115200);
  pc.printf("Hello world\n");

  uint8_t data[8] = {0};

  port.reset();
  port.frequency(125000);

  while (true) {
    data[0]++;
    led = !led;

    CANMessage msg(0x123, (char*)data, 8, CANData, CANStandard);

    if(port.write(msg)) {
      pc.printf("Sent!\n");
    } else {
      pc.printf("Not sent! :(\n");
    }

    wait(1);
  }

  return 0;
}

Messages are transmitted correctly, checked with logic analyser and with a USB-CAN tool.

NVstore

#include <mbed.h>
#include <nvstore.h>
#include <string.h>
#include <stdlib.h>
#include <stdio.h>

Serial     pc(PA_2, PA_3);

// Entry point for the example
int main() {
    pc.baud(115200);
    printf("\n--- Mbed OS NVStore example ---\n");
#if NVSTORE_ENABLED

    uint16_t actual_len_bytes = 0;

    // NVStore is a sigleton, get its instance
    NVStore &nvstore = NVStore::get_instance();

    int rc;
    uint16_t key;

    // Values read or written by NVStore need to be aligned to a uint32_t address (even if their sizes
    // aren't)
    uint32_t value;

    // Initialize NVstore. Note that it can be skipped, as it is lazily called by all other APIs
    rc = nvstore.init();
    printf("Init NVStore. ");
    printf("Return code is %d\n", rc);

    // Show NVStore size, maximum number of keys and area addresses and sizes
    printf("NVStore size is %d\n", nvstore.size());
    printf("NVStore max number of keys is %d (out of %d possible ones in this flash configuration)\n",
            nvstore.get_max_keys(), nvstore.get_max_possible_keys());
    printf("NVStore areas:\n");
    for (uint8_t area = 0; area < NVSTORE_NUM_AREAS; area++) {
        uint32_t area_address;
        size_t area_size;
        nvstore.get_area_params(area, area_address, area_size);
        printf("Area %d: address 0x%08lx, size %d (0x%x)\n", area, area_address, area_size, area_size);
    }

    // Clear NVStore data. Should only be done once at factory configuration
    rc = nvstore.reset();
    printf("Reset NVStore. ");
    printf("Return code is %d\n", rc);

    // Now set some values to the same key
    key = 1;

    value = 1000;
    rc = nvstore.set(key, sizeof(value), &value);
    printf("Set key %d to value %ld. ", key, value);
    printf("Return code is %d\n", rc);

    value = 2000;
    rc = nvstore.set(key, sizeof(value), &value);
    printf("Set key %d to value %ld. ", key, value);
    printf("Return code is %d\n", rc);

    value = 3000;
    rc = nvstore.set(key, sizeof(value), &value);
    printf("Set key %d to value %ld. ", key, value);
    printf("Return code is %d\n", rc);

    // Get the value of this key (should be 3000)
    rc = nvstore.get(key, sizeof(value), &value, actual_len_bytes);
    printf("Get key %d. Value is %ld. ", key, value);
    printf("Return code is %d\n", rc);

    // Now remove the key
    rc = nvstore.remove(key);
    printf("Delete key %d. ", key);
    printf("Return code is %d\n", rc);

    // Get the key again, now it should not exist
    rc = nvstore.get(key, sizeof(value), &value, actual_len_bytes);
    printf("Get key %d. ", key);
    printf("Return code is %d\n", rc);

    key = 12;

    // Now set another key once (it can't be set again)
    value = 50;
    rc = nvstore.set_once(key, sizeof(value), &value);
    printf("Set key %d once to value %ld. ", key, value);
    printf("Return code is %d\n", rc);

    // This should fail on key already existing
    value = 100;
    rc = nvstore.set(key, sizeof(value), &value);
    printf("Set key %d to value %ld. ", key, value);
    printf("Return code is %d\n", rc);

    // Get the value of this key (should be 50)
    rc = nvstore.get(key, sizeof(value), &value, actual_len_bytes);
    printf("Get key %d. Value is %ld. ", key, value);
    printf("Return code is %d\n", rc);

    // Get the data size for this key (should be 4)
    rc = nvstore.get_item_size(key, actual_len_bytes);
    printf("Data size for key %d is %d. ", key, actual_len_bytes);
    printf("Return code is %d\n", rc);
#else
    printf("NVStore is disabled for this board\n");
#endif

    printf("\n--- Mbed OS NVStore example done. ---\n");
}

NVstore functionality works as expected. The NVstore library is not enabled by default in the PlatformIO toolchain, I've communicated this to the PIO authors via a separate issue.

Async serial

// https://os.mbed.com/teams/SiliconLabs/code/Serial-LowPower-Demo/file/tip/main.cpp/
#include <mbed.h>

/*------------ Constant definitions --------------*/
#define TX_PIN          PA_2
#define RX_PIN          PA_3
#define BRATE           115200
#define LED_PIN         PB_12
#define TOGGLE_RATE     (0.5f)
#define BUFF_LENGTH     5

/*-------- Check if platform compatible ----------*/
#if DEVICE_SERIAL_ASYNCH
Serial test_connection(USBTX, USBRX);
#else
#error "Platform not compatible with Low Power APIs for Serial"
#endif

/*------------------ Variables -------------------*/
Ticker              blinker;
bool                blinking = false;
event_callback_t    serialEventCb;
DigitalOut          LED(LED_PIN);
uint8_t             rx_buf[BUFF_LENGTH + 1];

/*------------------ Callbacks -------------------*/
void blink(void) {
    LED = !LED;
}

/**
* This is a callback! Do not call frequency-dependent operations here.
*
* For a more thorough explanation, go here:
* https://developer.mbed.org/teams/SiliconLabs/wiki/Using-the-improved-mbed-sleep-API#mixing-sleep-with-synchronous-code
**/
void serialCb(int events) {
    /* Something triggered the callback, either buffer is full or 'S' is received */
    unsigned char i;
    if(events & SERIAL_EVENT_RX_CHARACTER_MATCH) {
        //Received 'S', check for buffer length
        for(i = 0; i < BUFF_LENGTH; i++) {
            //Found the length!
            if(rx_buf[i] == 'S') break;
        }

        // Toggle blinking
        if(blinking) {
            blinker.detach();
            blinking = false;
        } else {
            blinker.attach(blink, TOGGLE_RATE);
            blinking = true;
        }
    } else if (events & SERIAL_EVENT_RX_COMPLETE) {
        i = BUFF_LENGTH - 1;
    } else {
        rx_buf[0] = 'E';
        rx_buf[1] = 'R';
        rx_buf[2] = 'R';
        rx_buf[3] = '!';
        rx_buf[4] = 0;
        i = 3;
    }

    // Echo string, no callback
    test_connection.write(rx_buf, i+1, 0, 0);

    // Reset serial reception
    test_connection.read(rx_buf, BUFF_LENGTH, serialEventCb, SERIAL_EVENT_RX_ALL, 'S');
}

/*-------------------- Main ----------------------*/
int main() {
    /* Very Simple Main (tm) */
    serialEventCb.attach(serialCb);

    /* Setup serial connection */
    test_connection.baud(BRATE);
    test_connection.printf("Low Power API test\n\nSend 'S' to toggle blinking\n");
    test_connection.read(rx_buf, BUFF_LENGTH, serialEventCb, SERIAL_EVENT_RX_ALL, 'S');

    /* Let the callbacks take care of everything */
    while(1) sleep();
}

Works as expected.

Serial hardware flow control

#include <mbed.h>

DigitalOut led(PB_12);
Serial     pc(PA_2, PA_3);
int        counter = 0;

int main() {
  pc.baud(115200);

  pc.set_flow_control(Serial::RTSCTS, PA_1, PA_0);

  while (true) {
    counter++;
    pc.printf("Hello world: %d\n", counter);
    wait(1);
  }

  return 0;
}

Tested with a CP2104 USB-UART bridge and PySerial miniterm with --rtscts flag enabled. Works as expected.

Analog in

#include <mbed.h>

DigitalOut led(PB_12);
Serial     pc(PA_2, PA_3);

AnalogIn   adc(ADC_VREF);

int main() {
  pc.baud(115200);

  while (true) {
    pc.printf("ADC value: %f\n", adc.read());
    wait(1);
  }

  return 0;
}

The following pins were tested one-by-one:

  • PA_0, PA_1, PA_4, PA_5, PA_6, PA_7
  • PB_0, PB_1
  • ADC_TEMP, ADC_VREF

Pins PA_2 and PA_3 were not tested as they were used to communicate with the host computer.

PWM out

#include <mbed.h>

PwmOut pwm(PB_15);

int main() {
  while (true) {
    pwm = 0.25;
    wait(0.2);
    pwm = 0.75;
    wait(0.2);
  }

  return 0;
}

The following pins were tested and verified using a logic analyser:

  • PA_0, PA_1, PA_2, PA_3, PA_6, PA_7, PA_8, PA_9, PA_10, PA_11, PA_15
  • PB_0, PB_1, PB_3, PB_4, PB_5, PB_10, PB_11, PB_13, PB_14, PB_15

Note: The generated PWM pinmap for PA_7 did not work, I switched it to the ALT0 definition.

@cmonr cmonr added needs: review and removed needs: work labels Aug 31, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 31, 2018

@ARMmbed/team-st-mcd When y'all get a chance.

@jeromecoutant

Approved only from code review. No test

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 6, 2018

@0xc0170 0xc0170 changed the title from STM32: Corrected device_has_add flags for bluepill_f103c8 target, fixes #7654 to STM32: Correct device_has_add flags for bluepill_f103c8 target, fixes #7654 Sep 6, 2018

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 6, 2018

@0xc0170

The code looks good.

One change request: te file PeripheralPins.c 100644 → 100755 changes file permissions, please revert it

@cesarvandevelde

This comment has been minimized.

Contributor

cesarvandevelde commented Sep 6, 2018

Woops! No idea how that snuck in there, but should be fixed now...

@@ -37,6 +37,13 @@
extern "C" {
#endif
typedef enum {

This comment has been minimized.

@0xc0170

0xc0170 Sep 6, 2018

Member

also please fix file permission here for this file (I've missed it in the previous review)

@cesarvandevelde

This comment has been minimized.

Contributor

cesarvandevelde commented Sep 6, 2018

Done!

@0xc0170

0xc0170 approved these changes Sep 6, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 6, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 10, 2018

Build : SUCCESS

Build number : 3045
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7700/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 10, 2018

/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 11, 2018

Test CI needs to be restarted. Will restart aftetr 5.10-rc2 is generated.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 17, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 5391a7b into ARMmbed:master Sep 18, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 598 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9146 cycles (-81 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment