Fix PWM API for reading period and pulsewidth on LPC1768 (#274)

* Fix PWM API for reading microseconds on LPC1768

* Fix semihosting related compile errors, fix pwm deinit

* Fix some mistakes, fix PWM frequency adjustment

* Fix a few more issues

hal/tests/TESTS/mbed_hal/sleep/main.cpp

@@ -229,7 +229,7 @@ utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
 #endif
     ticker_suspend(get_us_ticker_data());
 
-#ifdef DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     // Disconnect semihosting now, because otherwise it will get disconnected on the first sleep call and
     // cause said call to take several milliseconds, leading to a test failure.
     mbed_interface_disconnect();

platform/include/platform/mbed_interface.h

@@ -47,7 +47,7 @@
 extern "C" {
 #endif
 
-#if DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
 
 /**
  * \defgroup platform_interface interface functions

platform/source/mbed_interface.c

@@ -19,7 +19,7 @@
 
 #include "platform/mbed_semihost_api.h"
 
-#if DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
 
 // return true if a debugger is attached, indicating mbed interface is connected
 int mbed_interface_connected(void)
@@ -90,7 +90,7 @@ WEAK int mbed_uid(char *uid)
 
 WEAK void mbed_mac_address(char *mac)
 {
-#if DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     char uid[DEVICE_ID_LENGTH + 1];
     int i;
 
@@ -115,7 +115,7 @@ WEAK void mbed_mac_address(char *mac)
         mac[3] = 0xF0;
         mac[4] = 0x00;
         mac[5] = 0x00;
-#if DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     }
 #endif
 }

platform/source/mbed_retarget.cpp

@@ -1570,7 +1570,7 @@ extern "C" void exit(int return_code)
     fsync(STDERR_FILENO);
 #endif
 
-#if DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     if (mbed_interface_connected()) {
         semihost_exit();
     }

rtos/tests/TESTS/mbed_rtos/MemoryPool/main.cpp

@@ -676,7 +676,7 @@ utest::v1::status_t greentea_test_setup(const size_t number_of_cases)
 {
     GREENTEA_SETUP(20, "default_auto");
 
-#ifdef DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     // Disconnect semihosting now, because otherwise it will get disconnected on the first sleep call and
     // cause said call to take several milliseconds, leading to a test failure.
     mbed_interface_disconnect();

rtos/tests/TESTS/mbed_rtos/mail/main.cpp

@@ -479,7 +479,7 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
 {
     GREENTEA_SETUP(10, "default_auto");
 
-#ifdef DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     // Disconnect semihosting now, because otherwise it will get disconnected on the first sleep call and
     // cause said call to take several milliseconds, leading to a test failure.
     mbed_interface_disconnect();

rtos/tests/TESTS/mbed_rtos/queue/main.cpp

@@ -319,7 +319,7 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
 {
     GREENTEA_SETUP(5, "default_auto");
 
-#ifdef DEVICE_SEMIHOST
+#if MBED_CONF_TARGET_SEMIHOSTING_ENABLED
     // Disconnect semihosting now, because otherwise it will get disconnected on the first sleep call and
     // cause said call to take several milliseconds, leading to a test failure.
     mbed_interface_disconnect();

targets/TARGET_NXP/TARGET_LPC176X/TARGET_MBED_LPC1768/PinNames.h

@@ -70,18 +70,6 @@ typedef enum {
     p29 = P0_5,
     p30 = P0_4,
 
-    // Other mbed Pin Names
-#ifdef MCB1700
-    LED1 = P1_28,
-    LED2 = P1_29,
-    LED3 = P1_31,
-    LED4 = P2_2,
-#else
-    LED1 = P1_18,
-    LED2 = P1_20,
-    LED3 = P1_21,
-    LED4 = P1_23,
-#endif
     CONSOLE_TX = P0_2,
     CONSOLE_RX = P0_3,
 
@@ -111,18 +99,25 @@ typedef enum {
     A5 = P1_31,
 
     // Not connected
-    NC = (int)0xFFFFFFFF,
-
-    I2C_SCL0 = NC,
-    I2C_SDA0 = NC,
-    I2C_SCL1 = p10,
-    I2C_SDA1 = p9,
-    I2C_SCL2 = P0_11, // pin used by application board
-    I2C_SDA2 = P0_10, // pin used by application board
-    I2C_SCL = I2C_SCL2,
-    I2C_SDA = I2C_SDA2,
+    NC = (int)0xFFFFFFFF
 } PinName;
 
+// Standard buttons and LEDs
+#define LED1 P1_18
+#define LED2 P1_20
+#define LED3 P1_21
+#define LED4 P1_23
+
+// I2C pin names
+#define I2C_SCL0 NC
+#define I2C_SDA0 NC
+#define I2C_SCL1 p10
+#define I2C_SDA1 p9
+#define I2C_SCL2 P0_11 // pin used by application board
+#define I2C_SDA2 P0_10 // pin used by application board
+#define I2C_SCL I2C_SCL2
+#define I2C_SDA I2C_SDA2
+
 typedef enum {
     PullUp = 0,
     PullDown = 3,

targets/TARGET_NXP/TARGET_LPC176X/pwmout_api.c

@@ -19,6 +19,18 @@
 #include "cmsis.h"
 #include "pinmap.h"
 
+#include <math.h>
+#include <stdlib.h>
+
+// Change to 1 to enable debug prints of what's being calculated.
+// Must comment out the critical section calls in PwmOut to use.
+#define LPC1768_PWMOUT_DEBUG 0
+
+#if LPC1768_PWMOUT_DEBUG
+#include <stdio.h>
+#include <inttypes.h>
+#endif
+
 #define TCR_CNT_EN       0x00000001
 #define TCR_RESET        0x00000002
 
@@ -53,7 +65,7 @@ __IO uint32_t *PWM_MATCH[] = {
 
 #define TCR_PWM_EN       0x00000008
 
-static unsigned int pwm_clock_mhz;
+static unsigned int pwm_clocks_per_us;
 
 void pwmout_init(pwmout_t *obj, PinName pin)
 {
@@ -67,7 +79,8 @@ void pwmout_init(pwmout_t *obj, PinName pin)
     // ensure the power is on
     LPC_SC->PCONP |= 1 << 6;
 
-    // ensure clock to /4
+    // Set PWM clock to CCLK / 4.  This means that the PWM counter will increment every (4 / SystemCoreClock) seconds,
+    // or 41.7ns (1us/24) with default clock settings.
     LPC_SC->PCLKSEL0 &= ~(0x3 << 12);     // pclk = /4
     LPC_PWM1->PR = 0;                     // no pre-scale
 
@@ -77,7 +90,8 @@ void pwmout_init(pwmout_t *obj, PinName pin)
     // enable the specific PWM output
     LPC_PWM1->PCR |= 1 << (8 + pwm);
 
-    pwm_clock_mhz = SystemCoreClock / 4000000;
+    // Calculate microseconds -> clocks conversion, factoring in the prescaler of 4 we set earlier.
+    pwm_clocks_per_us = SystemCoreClock / 4 / 1000000;
 
     // default to 20ms: standard for servos, and fine for e.g. brightness control
     pwmout_period_ms(obj, 20);
@@ -89,7 +103,11 @@ void pwmout_init(pwmout_t *obj, PinName pin)
 
 void pwmout_free(pwmout_t *obj)
 {
-    // [TODO]
+    // From testing, it seems like if you just disable the output by clearing the the bit in PCR, the output can get stuck 
+    // at either 0 or 1 depending on what level the PWM was at when it was disabled.
+    // Instead, we just set the duty cycle of the output to 0 so that it's guaranteed to go low.
+    *obj->MR = 0;
+    LPC_PWM1->LER = 1 << obj->pwm;
 }
 
 void pwmout_write(pwmout_t *obj, float value)
@@ -101,17 +119,24 @@ void pwmout_write(pwmout_t *obj, float value)
     }
 
     // set channel match to percentage
-    uint32_t v = (uint32_t)((float)(LPC_PWM1->MR0) * value);
+    uint32_t v = (uint32_t)lroundf((float)(LPC_PWM1->MR0) * value);
 
-    // workaround for PWM1[1] - Never make it equal MR0, else we get 1 cycle dropout
+    // workaround for the LPC1768 PWM1[1] errata - bad stuff happens for output 1 if the MR register equals the MR0 register
+    // for the module (this could happen if the user tries to set exactly 100% duty cycle).  To avoid this, 
+    // if the match register value would equal MR0, increment it again to make it greater than MR0.  This doesn't
+    // cause any side effects so long as we make sure MR0 is less than UINT32_MAX - 1.
     if (v == LPC_PWM1->MR0) {
         v++;
     }
 
     *obj->MR = v;
 
+#if LPC1768_PWMOUT_DEBUG
+    printf("Calculated MR=%" PRIu32 " for duty cycle %.06f (MR0 = %" PRIu32 ")\n", v, value, LPC_PWM1->MR0);
+#endif
+
     // accept on next period start
-    LPC_PWM1->LER |= 1 << obj->pwm;
+    LPC_PWM1->LER = 1 << obj->pwm;
 }
 
 float pwmout_read(pwmout_t *obj)
@@ -133,22 +158,37 @@ void pwmout_period_ms(pwmout_t *obj, int ms)
 // Set the PWM period, keeping the duty cycle the same.
 void pwmout_period_us(pwmout_t *obj, int us)
 {
+    // If the passed value is larger than this, it will overflow the MR0 register and bad stuff will happen
+    const uint32_t max_period_us = (UINT32_MAX - 2) / pwm_clocks_per_us;
+
     // calculate number of ticks
-    uint32_t ticks = pwm_clock_mhz * us;
+    if((uint32_t)us > max_period_us)
+    {
+        us = max_period_us;
+    }
+    if(us < 1)
+    {
+        us = 1;
+    }
+    uint32_t new_mr0_val = pwm_clocks_per_us * us;
 
     // set reset
     LPC_PWM1->TCR = TCR_RESET;
 
-    // set the global match register
-    LPC_PWM1->MR0 = ticks;
+    // Scale the pulse width to preserve the duty ratio.  Must use 64-bit math as the numerator can fairly easily overflow 32 bits.
+    uint32_t new_mr_val = (*obj->MR * ((uint64_t)new_mr0_val)) / LPC_PWM1->MR0;
+#if LPC1768_PWMOUT_DEBUG
+    printf("Changing MR0 from %" PRIu32 " to %" PRIu32 ", changing MR from %" PRIu32 " to %" PRIu32 " to preserve duty cycle\n", LPC_PWM1->MR0, new_mr0_val, *obj->MR, new_mr_val);
+#endif
+    *obj->MR = new_mr_val;
 
-    // Scale the pulse width to preserve the duty ratio
-    if (LPC_PWM1->MR0 > 0) {
-        *obj->MR = (*obj->MR * ticks) / LPC_PWM1->MR0;
-    }
+    // set the global match register.  Note that based on testing we do *not* need to subtract 1 here,
+    // e.g. setting MR0 to 4 causes the PWM to reset every 4 clocks.  This appears to be because the internal
+    // counter starts at 1 from reset, not 0.
+    LPC_PWM1->MR0 = new_mr0_val;
 
     // set the channel latch to update value at next period start
-    LPC_PWM1->LER |= 1 << 0;
+    LPC_PWM1->LER = 1 << 0;
 
     // enable counter and pwm, clear reset
     LPC_PWM1->TCR = TCR_CNT_EN | TCR_PWM_EN;
@@ -156,7 +196,8 @@ void pwmout_period_us(pwmout_t *obj, int us)
 
 int pwmout_read_period_us(pwmout_t *obj)
 {
-    return (LPC_PWM1->MR0);
+    // Add half a us worth of clocks for correct integer rounding.
+    return (LPC_PWM1->MR0 + pwm_clocks_per_us/2) / pwm_clocks_per_us;
 }
 
 void pwmout_pulsewidth(pwmout_t *obj, float seconds)
@@ -172,9 +213,9 @@ void pwmout_pulsewidth_ms(pwmout_t *obj, int ms)
 void pwmout_pulsewidth_us(pwmout_t *obj, int us)
 {
     // calculate number of ticks
-    uint32_t v = pwm_clock_mhz * us;
+    uint32_t v = pwm_clocks_per_us * us;
 
-    // workaround for PWM1[1] - Never make it equal MR0, else we get 1 cycle dropout
+    // workaround for PWM1[1] - Never make it equal MR0, else we get 1 cycle dropout.  See pwmout_write() for more details.
     if (v == LPC_PWM1->MR0) {
         v++;
     }
@@ -183,12 +224,18 @@ void pwmout_pulsewidth_us(pwmout_t *obj, int us)
     *obj->MR = v;
 
     // set the channel latch to update value at next period start
-    LPC_PWM1->LER |= 1 << obj->pwm;
+    LPC_PWM1->LER = 1 << obj->pwm;
 }
 
 int pwmout_read_pulsewidth_us(pwmout_t *obj)
 {
-    return (*obj->MR);
+    uint32_t mr_ticks = *(obj->MR);
+    if(mr_ticks > LPC_PWM1->MR0)
+    {
+        mr_ticks = LPC_PWM1->MR0;
+    }
+    // Add half a us worth of clocks for correct integer rounding.
+    return (mr_ticks + pwm_clocks_per_us/2) / pwm_clocks_per_us;
 }
 
 const PinMap *pwmout_pinmap()

targets/TARGET_NXP/TARGET_LPC176X/sleep.c

@@ -27,7 +27,7 @@
 
 void hal_sleep(void) {
 
-#if (DEVICE_SEMIHOST == 1)
+#if (MBED_CONF_TARGET_SEMIHOSTING_ENABLED == 1)
     // ensure debug is disconnected
     mbed_interface_disconnect();
 #endif
@@ -70,7 +70,7 @@ void hal_sleep(void) {
 
 void hal_deepsleep(void) {
 
-#if (DEVICE_SEMIHOST == 1)
+#if (MBED_CONF_TARGET_SEMIHOSTING_ENABLED == 1)
     // ensure debug is disconnected
     mbed_interface_disconnect();
 #endif

tools/cmake/mbed_greentea.cmake

@@ -115,7 +115,7 @@ function(mbed_greentea_add_test)
     # the serial port.  Doing that type of reset also seems to give the Pitaya-Link probe trouble.
     # However, for targets which support semihosting (currently just LPC1768), we do need the reset as otherwise
     # semihosting stuff like localfilesystem won't work.
-    if(NOT "DEVICE_SEMIHOST=1" IN_LIST MBED_TARGET_DEFINITIONS)
+    if(NOT "MBED_CONF_TARGET_SEMIHOSTING_ENABLED=1" IN_LIST MBED_CONFIG_DEFINITIONS)
         list(APPEND MBED_HTRUN_ARGUMENTS --skip-reset)
     endif()