Skip to content

boards/microduino-corerf: Initial support#12411

Merged
maribu merged 9 commits intoRIOT-OS:masterfrom
benpicco:atmega128rfa1
Oct 17, 2019
Merged

boards/microduino-corerf: Initial support#12411
maribu merged 9 commits intoRIOT-OS:masterfrom
benpicco:atmega128rfa1

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 9, 2019

Contribution description

The Microduino CoreRF is an inexpensive breakout board for the ATmega128RFA1 MCU.

The ATmega128RFA1 is the predecessor of the ATmega256RFR2. The ATmega256/128/64RFR2 data sheet says

Backward compatibility of the ATmega256/128/64RFR2 to the ATmega128RFA1 is
provided in most cases. However some incompatibilities may exist.
The ATmega256/128/64RFR2 uses the same package as the ATmega128RFA1.

Further search in the data sheet shows that the incompatibilities only concern different ADC characteristics.

Testing procedure

The board itself is rather spartan. It only provides a reset button and a non-controllable power LED.

The RX/TX pins have to be connected to a user-provided serial adapter to provide a UART interface.

Flashing

The board comes pre-flashed with an Arduino compatible bootloader, but I managed to delete it.
I've then spent way too much time trying to re-flash it (especially since most instructions call for a special capacitor on the reset line) until I discovered that flashing works remarkably well with a FT232H USB dongle I happened to have - no extra components needed.

All guides on how to re-flash the Arduino bootloader recommend using the Arduino IDE - which does not seem to support the FT232H dongle.

Since it works well with plain avrdude I decided not to bother with the bootloader and instead enjoy the additional ROM space.

If required, I can see how I can restore the bootloader and flash using that.

Tests

Since the board is basically just a plain Atmega MCU, I just checked if stdio and timers work.

For this I flashed examples/default, tests/xtimer_msg and tests/xtimer_drift - they all behaved as expected.

I did not test the radio yet.

Issues/PRs references

#9172 will add support for the radio

@benpicco benpicco added Platform: AVR Platform: This PR/issue effects AVR-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Oct 9, 2019
@benpicco benpicco requested a review from maribu October 9, 2019 23:47
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise this looks fine to me. Just ordered two boards so that I can test. I guess this has to wait until the end of the soft feature freeze anyway, so hopefully the boards arrive soon and testing will not add additional delay.

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 10, 2019
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2019
@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 15, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected :-) Let me check that moving the CPU-ID to atmega_common does not have any unexpected side affects on the other ATmegas. Then I'll ACK

@maribu
Copy link
Member

maribu commented Oct 15, 2019

OK, the Arduino-Mega2560 still works fine. The test in tests/periph_cpuid works as expected on the microduino

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@benpicco
Copy link
Contributor Author

I'll still need to update some BOARD_INSUFFICIENT_MEMORY lists. But some test builds will fail already because of #12412 😉

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 15, 2019
@benpicco
Copy link
Contributor Author

I added the board to the BOARD_INSUFFICIENT_MEMORY lists. That revealed some more issues in Murdock for which I had to provide more !fixups.

Are you fine with those? Than I'll squash again.

@maribu
Copy link
Member

maribu commented Oct 16, 2019

Yes. please squash again. I'd like to test one final time before merging.

(@maribu: Note to myself: Add boards/common/atmega/Makefile.features to reduce code duplication.)

benpicco and others added 6 commits October 17, 2019 19:20
Both atmega128rfa1 and atmega256rfr2 implement it.
GPIO_UNDEF is already defined in periph_cpu_common.h, so the
redefinition is never used.
This prevents a conflict with an unrelated Atmel header file.
The SLEEP define collides with an Atmel header file.
Rename the define to resolve the conflict.
@benpicco
Copy link
Contributor Author

Rebased now that #12484 is merged.
#12485 eased the pain: I added the Makefile.ci target and then ran for i in *; do make -C $i Makefile.ci; done

This would generate fresh Makefile.cis based on the content of the old Makefile. find . -name Makefile.ci -size 37c -delete then helped to get rid of the empty ones.

Merging the Makefile.cis was then much easier than going through everything by hand again.

@benpicco benpicco force-pushed the atmega128rfa1 branch 2 times, most recently from f4e8fea to a5bf856 Compare October 17, 2019 17:48
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Feel free to hit the merge button in order to reduce the chance of a new merge conflict popping up.

@maribu maribu merged commit cf81dff into RIOT-OS:master Oct 17, 2019
@maribu
Copy link
Member

maribu commented Oct 17, 2019

The very second I wrote this, Murdock passed 🤣

@benpicco benpicco deleted the atmega128rfa1 branch October 17, 2019 19:19
@benpicco
Copy link
Contributor Author

Thank you for the review!
And it sounds like you even bought one of those devices yourself 😃

@maribu
Copy link
Member

maribu commented Oct 17, 2019

Two. I figured I sooner or later someone should test the driver for the 802.15.4 transceiver, and that will be more fun with two devices. (Even though I also have other boards with radios that should be compatible over the air.)

@aabadie
Copy link
Contributor

aabadie commented Oct 17, 2019

It seems 2 unrelated commits went through: d92e2b2 and 252f22b. They are harmless, so not a big deal, but we should be careful that it doesn't happen too often.

@maribu
Copy link
Member

maribu commented Oct 17, 2019

They were needed to resolve name conflicts

@maribu
Copy link
Member

maribu commented Oct 17, 2019

The commit messages also document this. So the commit were required to resolve compilation issues, relevant for this PR (as they resolve compilation issues), reasonable (only internal defines are changes and the bew names make sense) and properly documented (the commit messages are right to the point).

@aabadie
Copy link
Contributor

aabadie commented Oct 18, 2019

Indeed, I missed the detailed part of the commit messages. At first sight, this was surprising.

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

Labels

Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants