[riot-notifications] [RIOT-OS/RIOT] drivers/dfplayer: New driver for the DFPlayer Mini MP3 player (#12363)

benpicco notifications at github.com
Sun May 10 02:13:04 CEST 2020


@benpicco commented on this pull request.

That's a neat little device!
I guess that folder structure is given by the firmware on the player.

I doubt anyone will have the hardware to test this, but the driver looks good and is self contained and I trust your testing. I was a bit unlucky with AliExpress lately. 

Btw you accidentally committed `drivers/include/.dfplayer.h.swo` :wink: 

> +#include <stdint.h>
+#include <string.h>
+
+#include "dfplayer.h"
+#include "dfplayer_constants.h"
+#include "dfplayer_internal.h"
+#include "periph/gpio.h"
+#include "periph/uart.h"
+#include "thread.h"
+#include "xtimer.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+/**
+ * @brief   Initial value of the FCS

FCS?

> +        dev->sync = thread_getpid();
+        thread_sleep();
+        dev->sync = KERNEL_PID_UNDEF;

That's an interesting way to do that.
I would have just called `mutex_lock()` (`xtimer_mutex_lock_timeout()`) here and unlocked it from the interrupt.

> +    xtimer_t timer = {
+        .callback = _timeout_cb,
+        .arg = dev
+    };
+    xtimer_set(&timer, DFPLAYER_BOOTUP_TIME_MS * US_PER_MS);
+    dev->sync = thread_getpid();
+    thread_sleep();
+    dev->sync = KERNEL_PID_UNDEF;
+
+    /* In case no timeout occurred, stop timer */
+    xtimer_remove(&timer);

I think `xtimer_mutex_lock_timeout()` would be a lot shorter :wink: 

-- 
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/12363#pullrequestreview-408692535
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200509/bffefff7/attachment-0001.htm>


More information about the notifications mailing list