Skip to content

Conversation

@elexx
Copy link
Contributor

@elexx elexx commented Jul 19, 2020

The Serpente Boards are small low-cost Atmel SAMD21 based development boards by @arturo182

Signed-off-by: Alexander Falb fal3xx@gmail.com

@elexx elexx force-pushed the board/serpente branch 4 times, most recently from 4db0945 to 2d66527 Compare July 20, 2020 11:08
@ioannisg ioannisg added the Release Notes To be mentioned in the release notes label Jul 20, 2020
@ioannisg ioannisg added this to the v2.4.0 milestone Jul 20, 2020
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

overall looks fine to me

Copy link
Contributor

@nzmichaelh nzmichaelh left a comment

Choose a reason for hiding this comment

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

Nice! Also check out the new seeeduino_xiao config for a more recent SAMD21 port.

@elexx
Copy link
Contributor Author

elexx commented Jul 20, 2020

Thanks for the pointers to seeeduino_xiao, I'll have a look.
Do you want me to amend to my original commit or just add a 2nd commit with the requested changes?

@nzmichaelh
Copy link
Contributor

Thanks for the pointers to seeeduino_xiao, I'll have a look.
Do you want me to amend to my original commit or just add a 2nd commit with the requested changes?

Best practice is to amend / fixup and then do a force push. Make sure you resolve the comments first though, else Github seems to lose them.

@elexx
Copy link
Contributor Author

elexx commented Jul 22, 2020

I'm a bit lost regarding PWM.
Because the user LED is ACTIVE_LOW I'd like to set the PWM to PWM_POLARITY_INVERTED (is this the right approach?). However this does not seem to be supported by the driver or I'm doing it wrong:

pwmleds {
	compatible = "pwm-leds";
	red_led_pwm: pwm_led_0 {
		pwms = <&tcc0 0 PWM_POLARITY_INVERTED>;
	};
}

Results in errors during the build $ west build samples/basic/blinky_pwm --board=serpente -p:

[...snip...]
serpente.dts.pre.tmp:336.4-30: Warning (pwms_property): /pwmleds/pwm_led_0:pwms: cell 2 is not a phandle reference
serpente.dts.pre.tmp:335.26-337.5: Warning (pwms_property): /pwmleds/pwm_led_0: Missing property '#pwm-cells' in node /soc/interrupt-controller@e000e100 or bad phandle (referred from pwms[2])
devicetree error: <Node /soc/interrupt-controller@e000e100 in 'serpente.dts.pre.tmp'> lacks #pwm-cells
[...snip...]

@elexx
Copy link
Contributor Author

elexx commented Jul 26, 2020

PWM is now working, however the output is inverted (the LED on the Serpente is ACTIVE_LOW which is currently not supported by the DTS for SAM0-TCC-PWM). I'm working on another PR to add this feature.

@nzmichaelh
Copy link
Contributor

PWM is now working, however the output is inverted (the LED on the Serpente is ACTIVE_HIGH which is currently not supported by the DTS for SAM0-TCC-PWM). I'm working on another PR to add this feature.

Sounds good, I was about to do the same. Let me know if you run out of time and I can take over the PWM change.

@elexx
Copy link
Contributor Author

elexx commented Jul 28, 2020

The Serpente board has a very limited pin count, but the pins are well chosen. It is possible to have 2UART or 1UART+1SPI or 1I2C+1SPI or 1UART+1I2C and so on (see notes in schematics).

What is the best practice in Zephyr for the default sercom definition in the board DTS? The options I was thinking about are:

  • Having both available sercoms with status okay, but nothing else in the DTS + overlay for all tests. (Probably the "cleanest" and most time consuming solution I could think of)
  • Having some defaults like 1SPI+1I2C in the DTS + overlays for the UART tests (I'm not sure which pinout should be the default)
  • Remove the supported flags from yaml (This feels like a quick and dirty hack)

Are there any better ways I missed? Which one do you prefer?

Copy link
Member

@vanwinkeljan vanwinkeljan left a comment

Choose a reason for hiding this comment

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

Please move serpente.overlay files in samples and tests to a separate commit as it has nothing to do with the commit boards: Add support for the Serpente Board

@elexx
Copy link
Contributor Author

elexx commented Aug 4, 2020

Please move serpente.overlay files in samples and tests to a separate commit as it has nothing to do with the commit boards: Add support for the Serpente Board

The tests and samples won't compile without warning (buildkite will fail), if I remove the overlay files. I would also have to remove the supported field from serpente.yaml, to simply not compile and tests and samples. But then the serpente.yml would not be aligned with the docs that also state what is supported by the board/mcu. I would like to keep this in one PR, but what do you think?

@vanwinkeljan
Copy link
Member

The tests and samples won't compile without warning (buildkite will fail), if I remove the overlay files. I would also have to remove the supported field from serpente.yaml, to simply not compile and tests and samples. But then the serpente.yml would not be aligned with the docs that also state what is supported by the board/mcu. I would like to keep this in one PR, but what do you think?

It was just a request to move the overlays to a separate commit, not a separate PR (so one commit to introduce the board and then one commit to add the overlays)

@elexx
Copy link
Contributor Author

elexx commented Aug 4, 2020

It was just a request to move the overlays to a separate commit, not a separate PR (so one commit to introduce the board and then one commit to add the overlays)

Sorry, I misunderstood. I just splittet the commit into two.

@elexx elexx requested a review from vanwinkeljan August 4, 2020 08:24
@elexx
Copy link
Contributor Author

elexx commented Aug 4, 2020

How do you want me to proceed with the failing build_all tests? The serpente only supports 2 different interfaces out of [UART, SPI, I2C] simultaneously, but some tests use all 3 of them. What is the best practice in this scenario?

@pfalcon pfalcon removed their request for review August 5, 2020 08:38
@vanwinkeljan
Copy link
Member

How do you want me to proceed with the failing build_all tests? The serpente only supports 2 different interfaces out of [UART, SPI, I2C] simultaneously, but some tests use all 3 of them. What is the best practice in this scenario?

You could blacklist the board for the test that require the 3 interfaces. Have a look at tests/drivers/build_all/testcase.yaml, some of the test cases in there already blacklist (platform_exclude:) the frdm_kw41z board

@elexx
Copy link
Contributor Author

elexx commented Aug 9, 2020

I guess I messed up the review resolving. Are there still any changes pending that I missed?

@vanwinkeljan vanwinkeljan dismissed their stale review August 9, 2020 18:40

Display related changes addressed

Comment on lines 7 to 29
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't all these overlay settings in the main board dts? I know the sercom can be configured as i2c or uart, but once you design a board will you really switch between them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating this myself a few days ago. In the end, I choose to follow the official pinout diagram and GPIO configuration of the Serpente.
My justification for this was, that the Serpente is a quite small development/prototyping board without a specific use-case in mind. It has just a few pins which provide maximum flexibility - these exact pins are specifically choose, because they provide the most diverse config options (from PWM, touch sensitive inputs, i2c, spi to uart and so on - with only 6 pins!). I didn't want to force a default configuration on the user, which might be unexpected since the official documentation does not describe any default configs.

On the other hand I totally understand that cluttering the code with overlay files is not perfect at all. If you prefer a default configuration of the Serpente in Zephyr, maybe @arturo182 can suggest one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@elexx put this perfectly, the idea is to not force the user into any specific configuration. TBH, it's even hard to choose which should be the default cause it totally depends on the use-case. If possible, I'd prefer we keep it as flexible as we can.

Copy link
Member

Choose a reason for hiding this comment

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

Setting these in the board dts doesn't lock a user into a specific configuration, as you can still override them in an application overlay if desired. I'm not keen on having so many overlays for this board. Can you at least put the console uart in the board dts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. I just added a default configuration for the zephyr console and amended the changes to my last commit.

Because I was not able to find this in the documentation or in some other SAMDx based board dts: Is it possible to use the USB connection as zephyr console by default? Without using a sercom at all? I guess this would be the preferred way for the Serpente, because it frees up 2 pins. And imho it also reduces the entry barrier for people new to Zephyr, because only one cable and no additional hardware is required to get started with Zephyr and some quick printk/log debugging.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if there are any boards that currently do this, but yes it should be possible. I suggest we go ahead and merge this PR, then you can submit a follow-up PR

@elexx
Copy link
Contributor Author

elexx commented Aug 13, 2020

@nzmichaelh I'm sorry, but somewhere on the way to finalize this PR it seems like I forgot to resolve a change request from you. Is there still anything you want me to fix in this PR?

Aside from that, is there anything else I can do to help finish up this PR?

@MaureenHelm
Copy link
Member

@nzmichaelh please revisit

@elexx
Copy link
Contributor Author

elexx commented Sep 3, 2020

@nzmichaelh just a friendly reminder that this PR still needs your attention :-)

The Serpente Boards are small low-cost Atmel SAMD21 based
development boards by @arturo182

Signed-off-by: Alexander Falb <fal3xx@gmail.com>
Add the supported features section to  serpente.yaml and overlay files
to the corresponding tests and samples.

Signed-off-by: Alexander Falb <fal3xx@gmail.com>
@MaureenHelm MaureenHelm dismissed nzmichaelh’s stale review September 4, 2020 14:24

no response after multiple pings

@MaureenHelm MaureenHelm merged commit 402a352 into zephyrproject-rtos:master Sep 4, 2020
@elexx elexx deleted the board/serpente branch September 4, 2020 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Boards area: Devicetree area: Documentation area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants