-
Notifications
You must be signed in to change notification settings - Fork 296
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
device: add e104_bt5040u support #607
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! This overall looks good to me. I've left some comments for the parts I think must be addressed before merging. I'll also ask a review from @kaczmarczyck next week for coherence with the overall project.
@@ -58,6 +58,7 @@ You will need one the following supported boards: | |||
to have a more practical form factor. | |||
* [Makerdiary nRF52840-MDK USB dongle](https://wiki.makerdiary.com/nrf52840-mdk/). | |||
* [Feitian OpenSK dongle](https://feitiantech.github.io/OpenSK_USB/). | |||
* [Ebyte E104-BT5040U](https://ebyte.com/en/product-view-news.html?id=1185) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the existing list items:
* [Ebyte E104-BT5040U](https://ebyte.com/en/product-view-news.html?id=1185) | |
* [Ebyte E104-BT5040U](https://ebyte.com/en/product-view-news.html?id=1185) |
|
||
 | ||
|
||
In fact, this hardware is a cheaper implementation of the [Nordic nRF52840 Dongle](https://www.nordicsemi.com/Software-and-tools/Development-Kits/nRF52840-Dongle), and the vast majority of its circuit mapping is the same or similar to the , so the parameters of the commands below can be directly reused for the Nordic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why start with "In fact"?
|
||
 | ||
|
||
In fact, this hardware is a cheaper implementation of the [Nordic nRF52840 Dongle](https://www.nordicsemi.com/Software-and-tools/Development-Kits/nRF52840-Dongle), and the vast majority of its circuit mapping is the same or similar to the , so the parameters of the commands below can be directly reused for the Nordic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have references why it's cheaper? Otherwise I would omit this first sentence and just keep the part regarding its similarity with the Nordic dongle simplifying the setup.
|
||
 | ||
|
||
In fact, this hardware is a cheaper implementation of the [Nordic nRF52840 Dongle](https://www.nordicsemi.com/Software-and-tools/Development-Kits/nRF52840-Dongle), and the vast majority of its circuit mapping is the same or similar to the , so the parameters of the commands below can be directly reused for the Nordic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "the , so" seems to be missing a word
|
||
### Flashing using DFU (Not recommended) | ||
|
||
In fact, the DFU of the E104's nrf52840 chip is corrupted due to a factory firmware issue. You can try to reflash into [an available DFU bootloader](https://github.com/google/OpenSK/issues/545#issuecomment-1292411915) and then use that to flash into OpenSK again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why start with "In fact"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment on markdown, we wrap lines explicitly in the source and don't rely on automatic wrapping from source editors. (This is independent of markdown rendering which ignores source line wrapping when deciding what constitutes a paragraph.)
./deploy.py --board=nrf52840_dongle_dfu --opensk --programmer=nordicdfu | ||
``` | ||
|
||
The script will ask you to switch to DFU mode. To activate that on your dongle, keep the button pressed while inserting the device into your USB port. You may additionally need to press the tiny, sideways facing reset button. The device indicates DFU mode with a slowly blinking red LED. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of copy/pasting all of this from the dongle documentation, I would point to that documentation instead to avoid instructions duplication decreasing maintainability.
docs/img/e104_front.jpg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok to use this image? Or should we render it directly from its source instead of copying it in the repository?
@jmichelp Do you know?
Thanks! I'm happy to have more documentation about hardware options! I agree we shouldn't copy as much, and rather point to the Dongle while highlighting the differences:
Feel free to to copy out of my earlier comments. |
Closed for inactivity. Feel free to reopen when you decide to address our comments. |
Fixes #545