Skip to content
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

Add GD32C1xx support #53

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Add GD32C1xx support #53

merged 4 commits into from
Dec 14, 2023

Conversation

sparrowgrine
Copy link
Contributor

@sparrowgrine sparrowgrine commented Dec 10, 2023

This commit adds support for the GD32C103/GD32C113, and cleans up a few issues that svdrust brought up about various improper write constraints.

Copy link

github-actions bot commented Dec 10, 2023

Memory map comparison

Copy link
Collaborator

@qwandor qwandor left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It looks like the GD32C1x3 series is very similar to the GD32E1xx series; in fact the GigaDevice product selection guide lists them as being part of the same series. Do you think it would make sense to support them within the existing gd32e1 crate rather than in a separate gd32c1 crate? I guess the disadvantage is discoverability.

devices/gd32c103.yaml Outdated Show resolved Hide resolved
devices/gd32c113.yaml Outdated Show resolved Hide resolved
peripherals/can/can_c1.yaml Outdated Show resolved Hide resolved
# Copyright 2021 The gd32-rs authors.
#
# SPDX-License-Identifier: MIT OR Apache-2.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file looks very similar to can_common.yaml. Rather than duplicating it all, please include can_common.yaml here and then keep just the different parts in this file. If there are parts of can_common.yaml which don't apply then you can split them out to a new file which is included for the other models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file looks very similar to can_common.yaml. Rather than duplicating it all, please include can_common.yaml here and then keep just the different parts in this file. If there are parts of can_common.yaml which don't apply then you can split them out to a new file which is included for the other models.

The C1 series can peripheral has substantial differences, I plan on adding the CAN-FD specific registers in a future PR. For now, i just cloned can_common.yaml as a placeholder.

peripherals/i2c/i2c_e1.yaml Outdated Show resolved Hide resolved
@sparrowgrine
Copy link
Contributor Author

It looks like the GD32C1x3 series is very similar to the GD32E1xx series; in fact the GigaDevice product selection guide lists them as being part of the same series. Do you think it would make sense to support them within the existing gd32e1 crate rather than in a separate gd32c1 crate? I guess the disadvantage is discoverability.

yeah, my worry was discoverability. if it makes more sense to throw it in the gd32e1 crate thats fine, but it wont be the most intuitive thing from a user perspective.

Copy link
Collaborator

@qwandor qwandor left a comment

Choose a reason for hiding this comment

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

Thanks! Let me know when this is ready to release to crates.io.

@qwandor qwandor merged commit f7462e9 into gd32-rust:main Dec 14, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants