[riot-notifications] [RIOT-OS/RIOT] boards: jiminy-mega256rfr2: initial support (#7738)

Peter Kietzmann notifications at github.com
Thu Feb 1 10:22:44 CET 2018


PeterKietzmann requested changes on this pull request.

@shr70 @Josar I started a next review and testing round. Besides the commented STDIO baudrate, everything works rather stable now. Nice! Mainly I see style issue open. Please address my comments from yesterday before I look at the code again.

> +export CPU = atmega256rfr2
+
+USEMODLUE += periph
+
+# export needed for flash rule
+export PORT_LINUX ?= /dev/ttyACM0
+export PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))
+
+# Serial Baud rate for Ffasher is configured to 500kBaud
+# see /usr/include/asm-generic/termbits.h for availabel baudrates on your linux system
+export PROGRAMMER_SPEED ?= 0010005
+
+export FFLAGS += -p atmega256rfr2
+
+# refine serial port information for pyterm
+export BAUD = 250000

For me this does not run stable (38400 does). When using shell applications and typing a command, the last character is often ignored.

> +#include "cpu.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name   Baudrate for STDIO terminal
+ * possible Baudrate for board with good error rates
+ * 250000
+ * 38400
+ *
+ * Matche this with BAUD in Board/Makefile.include
+ * @{
+ */
+#define UART_STDIO_BAUDRATE (250000)       /**< Sets Baudrate for e.g. Shell */

If we reduce the baud rate again, don't forget this one.

> +#define TIMER_2             MEGA_TIMER1
+#define TIMER_2_MASK        &TIMSK1
+#define TIMER_2_FLAG        &TIFR1
+#define TIMER_2_ISRA        TIMER1_COMPA_vect
+#define TIMER_2_ISRB        TIMER1_COMPB_vect
+#define TIMER_2_ISRC        TIMER1_COMPC_vect
+/** @} */
+
+/**
+ * @name   PWM configuration
+ * @{
+ */
+
+/* To reduce confusion by steadily renaming hardware modules with different numbered drivers
+ * namingconvention is not to start with 0 but to name driver with the number fitting the underlying timer module*/
+#define PWM_1       (0U)

I still don't understand why you need to `#define PWM_1`. Please remove. I'm also a bit confused about the below configuration: which peripheral PWM driver did you use?

> +        .chan = { { .pin = GPIO_PIN(PORT_B, 5), .cc_chan = 0 },
+                  { .pin = GPIO_PIN(PORT_B, 6), .cc_chan = 1 },
+                  { .pin = GPIO_PIN(PORT_B, 7), .cc_chan = 2 } },
+        .scale_pointer = &(scale[0]),
+        .prescaler_pointer = &(prescaler[0]),
+        .bits = 16
+    }
+};
+#define PWM_NUMOF   (sizeof(pwm_config)/sizeof(pwm_conf_t))
+/** @} */
+
+/**
+ * @name Analog comparator configuration
+ * @{
+ */
+#define AC_0        (0U)

Address comment?

> + */
+#define AC_0        (0U)
+
+static const ac_conf_t ac_config[] = {
+    {
+        .in1 = GPIO_PIN(PORT_E, 2),
+        .in2 = GPIO_PIN(PORT_E, 1)
+    }
+};
+
+#define AC_NUMOF    (sizeof(ac_config)/sizeof(ac_conf_t))
+/*
+ * since current Analog comparator ISR is to slow, we'll hardcode it into the module
+ * and use the define to not include the line if the capacity_module isn't used
+ */
+#define USES_CAPACITY_MODULE

Here too?

> +*/
+uint8_t mcusr_mirror __attribute__((section(".noinit")));
+uint8_t soft_rst __attribute__((section(".noinit")));
+void get_mcusr(void) __attribute__((naked)) __attribute__((section(".init0")));
+void get_mcusr(void)
+{
+    /* save the reset flags passed from the bootloader */
+    __asm__ __volatile__("mov %0, r2\n" : "=r" (mcusr_mirror) :);
+    __asm__ __volatile__("mov %0, r3\n" : "=r" (soft_rst) :);
+}
+
+
+void _reset_cause(void){
+
+	if (mcusr_mirror & (1 << PORF)) {
+		DEBUG("Power-on reset.\n");

Please fix indentation

> +}
+
+
+void _reset_cause(void){
+
+	if (mcusr_mirror & (1 << PORF)) {
+		DEBUG("Power-on reset.\n");
+	}
+	if (mcusr_mirror & (1 << EXTRF)) {
+	    DEBUG("External reset!\n");
+	}
+	if (mcusr_mirror & (1 << BORF)) {
+	    DEBUG("Brownout reset!\n");
+	}
+	if (mcusr_mirror & (1 << WDRF)) {
+		if (soft_rst &  0xAA) {

Please fix indentation

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/7738#pullrequestreview-93206285
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20180201/dce2ca4d/attachment-0001.html>


More information about the notifications mailing list