Skip to content

Adding: boards/avr-rss2 and AtMega256RFR2 based board See doc.txt for details.#12668

Merged
benpicco merged 1 commit into
RIOT-OS:masterfrom
herjulf:avr-rss2-PR
Nov 20, 2019
Merged

Adding: boards/avr-rss2 and AtMega256RFR2 based board See doc.txt for details.#12668
benpicco merged 1 commit into
RIOT-OS:masterfrom
herjulf:avr-rss2-PR

Conversation

@herjulf

@herjulf herjulf commented Nov 7, 2019

Copy link
Copy Markdown
Contributor

Contribution description

Support for a new boards/avr-rss2 an AtMega256RFR2 based. See doc.txt for detail. PR does not touch any core files. The PR also demonstrates ease and small footprint in RIOT. PR uses in total 426 bytes.

Testing procedure

Tested with examples/gnrc_networking using the at86rf2xx radio with the recent PR #12537 using ping6 RPL over 6LowPAN.

Issues/PRs references

Follow-up work is planned to enable eui64 chip support see PR #12592

@herjulf

herjulf commented Nov 7, 2019

Copy link
Copy Markdown
Contributor Author

Tested with gnrc_networking and PR #12537 radio ping6 RPL. Planned integration with HW eui64 see PR #12598

@herjulf

herjulf commented Nov 7, 2019

Copy link
Copy Markdown
Contributor Author

Also a demonstration of board footprint in RIOT. In total only 426 bytes used here.

@benpicco benpicco added Area: boards Area: Board ports Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 7, 2019
@benpicco benpicco requested review from aabadie and maribu November 7, 2019 21:53

@benpicco benpicco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can also expose your LEDs and the Button through SAUL so users can interact with it on the command line.
You just have to add a gpio_params.h file and use the saul_gpio module.

Comment thread boards/avr-rss2/Makefile.dep Outdated
Comment thread boards/avr-rss2/Makefile.dep Outdated
Comment thread boards/avr-rss2/Makefile.include
Comment thread boards/avr-rss2/include/board.h Outdated
Comment thread boards/avr-rss2/include/board.h Outdated
@herjulf

herjulf commented Nov 7, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for review! Tried to address the comments in last the commit. More?

Comment thread boards/avr-rss2/Makefile.features Outdated
CPU = atmega256rfr2

# This board is based on an atmega CPU, thus import the features from it
include $(RIOTBOARD)/common/arduino-atmega/Makefile.features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this board a clone of an Arduino ATmega ? If no, I would just remove this line.

Comment thread boards/avr-rss2/Makefile.features
Comment thread boards/avr-rss2/board.c Outdated
LED_PORT |= (LED1_MASK | LED0_MASK);
}

void board_init(void)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function is not needed since your board is already using board_init from common/atmega: rename this led_init.c and remove the useless board_init implementation.

Comment thread boards/avr-rss2/board.c Outdated
#include "board.h"
#include "cpu.h"
#include "net/eui64.h"
#include <string.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These system includes doesn't seem required.

Comment thread boards/avr-rss2/board.c Outdated

#include "board.h"
#include "cpu.h"
#include "net/eui64.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove

Comment thread boards/avr-rss2/doc.txt Outdated
* Light Sensor.
* 32kHz RTC clock xtal
* HW comparator chip and input.
* Progammable power FET, (relay) for power on/off of sensors.

@aabadie aabadie Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codespell should report this kind of typo: progammable => programmable. There's another one on the same word below.

Comment thread boards/avr-rss2/doc.txt Outdated
for 3 seconds.

Flashing hello-wprld line example 256k MCU:
avrdude -p m256rfr2 -c stk500v2 -P /dev/ttyUSB0 -b 115200 -e -U flash:w:bin/rss2-mega256rfr2/hello-world.elf

@aabadie aabadie Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't make BOARD=avr-rss2 flash doing this ?

Comment thread boards/avr-rss2/include/board.h Outdated
* @{
*/
#ifndef STDIO_UART_BAUDRATE
#define STDIO_UART_BAUDRATE (115200) /**< Sets Baudrate for e.g. Shell */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Define not needed, it's already the default value for stdio_uart

Comment thread boards/avr-rss2/include/periph_conf.h Outdated
* @name RTC configuration
* @{
*/
#define RTC_NUMOF (1U)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Define is not needed.

Comment thread boards/avr-rss2/include/rss2.h Outdated
@@ -0,0 +1,39 @@

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please move these defines with proper doxygen documentation in board.h. Then remove rss2.h.

@aabadie

aabadie commented Nov 8, 2019

Copy link
Copy Markdown
Contributor

Please also fix the issues reported by Travis.

@herjulf

herjulf commented Nov 8, 2019

Copy link
Copy Markdown
Contributor Author

Thanks comments and improvements!
Most things should be addressed. So we give it a try.

@miri64

miri64 commented Nov 9, 2019

Copy link
Copy Markdown
Member

@herjulf please have a look at our contribution guidelines regarding formatting of commit and PR summaries.

@maribu

maribu commented Nov 12, 2019

Copy link
Copy Markdown
Member

@herjulf: I changed the PR description, as you accidentally entered the details between <!-- and -->. This resulted in your text being treated as comments and hidden in the web site.

@herjulf

herjulf commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

Thanks a lot! My first PR just from the command line so more confused than usual. I'll push a better approximation for doxygen to see if travis gets happier.

@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 Nov 13, 2019
@herjulf herjulf force-pushed the avr-rss2-PR branch 2 times, most recently from e6f3e11 to 04f24b1 Compare November 13, 2019 21:03
@herjulf

herjulf commented Nov 14, 2019

Copy link
Copy Markdown
Contributor Author

Thanks!. Pushing the changes.

Comment thread boards/avr-rss2/Makefile.dep
Comment thread boards/avr-rss2/doc.txt

### Flashing/Programming hardware
Programming using avrdude using serial bootloader using a TTL-USB cable.
Press the RESET button. The bootloader with wait for boot commands for 3 seconds. Program with:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Press the RESET button.

You should highlight this more.
With most boards that are flashed over UART the reset is triggered automatically by e.g. the CTS line.

At first I thought I connected it wrong or broke it by using 5V Vcc, but then I read the docs more carefully again.

Probably doesn't hurt to mention that the Vcc pin on the FTDI header is connected to the LDO, so supplying 5V is perfectly fine.

Comment thread boards/avr-rss2/include/board.h
Comment thread boards/avr-rss2/include/board.h Outdated
Comment on lines +38 to +39
#define LED0_PIN (1 << PE4) /* RED */
#define LED1_PIN (1 << PE3) /* YELLOW */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are those the same on my revision of the board?
when I do saul write 1 1 in examples/default I'd expect the LED to light up.

Comment thread boards/avr-rss2/Makefile.dep Outdated
USEMODULE += boards_common_atmega

ifneq (,$(filter saul_default,$(USEMODULE)))
USEMODULE += bmx280

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
USEMODULE += bmx280
USEMODULE += bme280

bme280 is a pseudo-module that selects the bmx280 driver.
Also it looks like it doesn't have the driver-default I2C address of 0x77, so you'll have to define BMX280_PARAM_I2C_ADDR in board.h

i2c_scan reports

2019-11-16 00:22:49,213 #  i2c_scan 0
2019-11-16 00:22:49,216 # Scanning I2C device 0...
2019-11-16 00:22:49,222 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-11-16 00:22:49,225 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-11-16 00:22:49,228 # 0x00 R R R R R R R R R R R R R R - -
2019-11-16 00:22:49,231 # 0x10 - - - - - - - - - - - - - - - -
2019-11-16 00:22:49,236 # 0x20 - - - - - - - - - - - - - - - -
2019-11-16 00:22:49,239 # 0x30 X - - - - - - - - - - - - - - -
2019-11-16 00:22:49,242 # 0x40 - - - - - - - - - - - - - - - -
2019-11-16 00:22:49,245 # 0x50 X - - - - - - - X - - - - - - -
2019-11-16 00:22:49,248 # 0x60 - - - - - - - - - - - - - - - -
2019-11-16 00:22:49,253 # 0x70 - - - - - - - - R R R R R R R R

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using the 2.4 Rev board? Should on 77. I see with
tests/driver_bmx280:

Temperature [°C]: 24.0
Pressure [Pa]: 101301
Humidity [%rH]: 46.72

In Contiki avr-rss2 port
#define BME280_ADDR (0x77 << 1) /* Alternative 0x76 */

BTW. There is a indoor AQ variant BME680. It can be soldered as a replacement for BME280. I did have sort of driver in Contiki. But the mapping to actual AQ was not knot known as that time.

@benpicco benpicco Nov 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah I got the 2.3a revision of the board.

0x58 is the EEPROM - what is 0x50 and 0x30?

edit: I just checked the data sheet - AT24MAC has two addresses, 0x50 for the EEPROM and 0x58 for the EUI functionality.

Still wondering what 0x30 is then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AT24MAC has address selections pin. In avr-rss2 EUI chip get's 0xB0 and the EEPROM should get 0XA0 but I've never used the EEPROM actually you pointed out it was on another address. For 0x30 I'm confused. Where can I find the i2_scan your're using?

@benpicco benpicco Nov 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where can I find the i2_scan your're using?

In e.g. examples/default add USEMODULE += i2c_scan to your Makefile.
Then it's available as a shell command.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@benpicco You had another AtMega256RFR2 board? Can you run i2c_scan on this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have a Microduino CoreRF with the ATmega128RFA1. It's basically just a breakout for the MCU with nothing connected but the bare necessities.
There i2c_scan prints

2019-11-18 13:46:59,160 # Scanning I2C device 0...
2019-11-18 13:46:59,239 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-11-18 13:46:59,240 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-11-18 13:46:59,241 # 0x00 R R R R R R R R R R R R R R - -
2019-11-18 13:46:59,242 # 0x10 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,243 # 0x20 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,244 # 0x30 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,245 # 0x40 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,246 # 0x50 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,247 # 0x60 - - - - - - - - - - - - - - - -
2019-11-18 13:46:59,248 # 0x70 - - - - - - - - R R R R R R R R

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
I desoldered the 24MAC602 chip

0x00 R R R R R R R R R R R R R R - -
0x10 - - - - - - - - - - - - - - - -
0x20 - - - - - - - - - - - - - - - -
0x30 - - - - - - - - - - - - - - - -
0x40 - - - - - - - - - - - - - - - -
0x50 - - - - - - - - - - - - - - - -
0x60 - - - - - - - - - - - - - - - -
0x70 - - - - - - - - R R R R R R R R

Same result. So it's the address chip. And reading the chip doc carefully on page 15. Chapter 11. Write protection:

Once the permanent software write protection
has been enabled, the device will no longer acknowledge the ‘0110’ (6h) control byte and cannot be reversed
even if the device is powered down.

So 0x30 addr will dissapear when EEPROM becomes write protected. So I think we solved this...

@benpicco benpicco Nov 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alright, thank you for clearing this up.
So what about the LEDs? Are they wired different in version 2.3a?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes LED has same wiring. Actually the rss2.h I was asked to remove has very concise summary of the boards.

@herjulf

herjulf commented Nov 18, 2019

Copy link
Copy Markdown
Contributor Author

EU requires all radio equipment to be approved by Radio Equipment Directive (RED) IoT stuff was a driving force behind this. Products having some kind of radio and put on the market must has this approval. Before RED the directive was R&TTE. Not fun but that's the way it is. Rev 2.3 was R&TTE Rev 2.4 is RED. So we can forget Rev 2.3.

Comment thread boards/avr-rss2/include/board.h Outdated
Comment thread boards/avr-rss2/include/board.h Outdated
Comment thread boards/avr-rss2/include/board.h Outdated
Comment thread boards/avr-rss2/include/board.h Outdated
Comment thread boards/avr-rss2/include/board.h Outdated
@herjulf

herjulf commented Nov 19, 2019

Copy link
Copy Markdown
Contributor Author

A bit off-topic but bme680 is in PR #12717. As said it can be mounted instead of bme280 for indoor AQ.

Comment thread boards/avr-rss2/Makefile.dep Outdated
USEMODULE += boards_common_atmega

ifneq (,$(filter saul_default,$(USEMODULE)))
USEMODULE += bmx280

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
USEMODULE += bmx280
USEMODULE += bme280

bmx280 is the generic driver, depending on whether you use USEMODULE += bme280 or USEMODULE += bmp280 different code paths are activated by #ifdefs in the driver.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. OK I see now. The driver is also checking chip-id. So it's a "feature" of the driver allow inconsistency. Yes patch needed. Anything more?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually we should probably add a warning if a pseudomodule is used directly…

@benpicco

Copy link
Copy Markdown
Contributor

With the USEMODULE change this should be good.
Please squash directly.

@benpicco

Copy link
Copy Markdown
Contributor

A bit off-topic but bme680 is in PR #12717. As said it can be mounted instead of bme280 for indoor AQ.

@herjulf you can always help advancing a PR by testing it or adding your comments :)

@herjulf

herjulf commented Nov 19, 2019

Copy link
Copy Markdown
Contributor Author

@benpicco Yes there are many interesting PR in the queue. EUI64, async timer stuff, BME680, RDC. But also RFID are current interests. PM-sensors we're using and have in ported to Contiki. But this focus right now.

@herjulf

herjulf commented Nov 19, 2019

Copy link
Copy Markdown
Contributor Author

The tests/leds is not working until led_init is added
also need a to declared.
board_init() was removed during the review process.
cpu/atmega_common/startup.c: board_init();

Seems like this has to be added and execute led_init()
Or any other suggestion?

@benpicco

Copy link
Copy Markdown
Contributor

Uh this is actually a bug that got introduced recently when common code for the ATmega boards got factored out - will provide a PR with the fix.

Just one more nit: Please reword your commit message so it's consistent with the coding conventions.

Something like boards/avr-rss2: add AtMega256RFR2 based board would do.

Co-Authored-By: benpicco <benpicco@googlemail.com>
Support: Alexandre Abadie aabadie, Marian Buschsieweke maribu, Martine Lenders miri64
@herjulf

herjulf commented Nov 20, 2019

Copy link
Copy Markdown
Contributor Author

Fine. Happy to see #12745. I'll try a PR renaming,

@benpicco benpicco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks fine and tested on the board, this should be good.

@benpicco benpicco merged commit c4d905f into RIOT-OS:master Nov 20, 2019
@herjulf

herjulf commented Nov 20, 2019

Copy link
Copy Markdown
Contributor Author

Thanks a lot! For the code review and the intro to the RIOT world and eco-system. First a beer. Later, at least my plan is to find a way to get the HW EUI64 get out.

@benpicco

Copy link
Copy Markdown
Contributor

Later, at least my plan is to find a way to get the HW EUI64 get out.

#12746 should address that, the interesting part is how to wire it up with the rest of the stack so it get's used automatically when available.

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

Labels

Area: boards Area: Board 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 Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants