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 basic Charlieplexing scan #10

Merged
merged 2 commits into from
Jun 6, 2024
Merged

Conversation

kienvo
Copy link
Member

@kienvo kienvo commented May 19, 2024

Hey, I just finished some basic initial code for the project.
There are some problems with the interrupt when using xpack's riscv-none-elf-gcc:

  • The gpio interrupt handler only runs once
  • The timer interrupt handler is so slow

Switching back to riscv-none-embed-gcc seems working, but the timer interrupt handler takes so much time that the main loop doesn't have enough time slice to execute.

Main changes:

  • Generate code from SDK
  • Add basic Charlieplexing scan

Makefile Outdated Show resolved Hide resolved
@kienvo kienvo requested a review from fcartegnie May 20, 2024 06:23
src/leddrv.c Outdated
@@ -3,46 +3,61 @@
#define LED_DRIVE_STRENTH GPIO_ModeOut_PP_20mA
#define LED_PINCOUNT (23)

#define PA_BASE ((uint32_t *)0x400010A0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if we are going to access non 4 bytes aligned offsets, but in that case, it will deref the wrong address

Copy link
Member Author

@kienvo kienvo May 29, 2024

Choose a reason for hiding this comment

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

There is no access to non 4 bytes aligned offsets. But I've just found pre-defined base address in the StdPeriphDriver; I'll use that instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you just changed the code, so I guess you understood the issue with gpio_base being already uint32_t aligned

*(uint32_t *)(gpio_base + GPIO_PD_DRV) = *cfg;

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a workaround for that before the last change:

uint32_t b = (uint32_t)gpio_base; // <--here
*(uint32_t *)(b + GPIO_PD_DRV) = *cfg;

It seems kind of ugly but I changed to use pre-defined base address in the last change anyway.

Copy link
Collaborator

@fcartegnie fcartegnie left a comment

Choose a reason for hiding this comment

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

that is not matching coding style and unneeded

@kienvo
Copy link
Member Author

kienvo commented May 31, 2024

that is not matching coding style and unneeded

Did you mean commit 817425b? It seem nonsense but for quick testing. I'll remove it here and add changing brightness feature in another PR. How do you think?

@fcartegnie
Copy link
Collaborator

Makefile changes

@kienvo
Copy link
Member Author

kienvo commented May 31, 2024

Makefile changes

954f786
4cece4e
d812e56
208b854
Which one?

@fcartegnie
Copy link
Collaborator

Makefile changes

4cece4e

@kienvo
Copy link
Member Author

kienvo commented May 31, 2024

What coding style do you suggest?

@kienvo kienvo requested a review from fcartegnie May 31, 2024 12:00
@kienvo
Copy link
Member Author

kienvo commented May 31, 2024

I see many projects use the old style, but isn't it convenient to have this appending style? It could be

  • turned on/off a flag by commenting out a line
  • or add an end-line comment without affecting other flags.

@kienvo
Copy link
Member Author

kienvo commented Jun 2, 2024

@fcartegnie if there is no other change I think it is time to get this PR merge.

@kienvo kienvo marked this pull request as ready for review June 3, 2024 01:10
@fcartegnie
Copy link
Collaborator

@fcartegnie if there is no other change I think it is time to get this PR merge.

You can't merge do & undos or fixs to those commits.

Please rebase -i and discard reverted and reverter code, and merge intra fix changes

@kienvo
Copy link
Member Author

kienvo commented Jun 3, 2024

You can't merge do & undos or fixs to those commits.

Please rebase -i and discard reverted and reverter code, and merge intra fix changes

Then force push or should I make a new PR?

@fcartegnie
Copy link
Collaborator

You can't merge do & undos or fixs to those commits.
Please rebase -i and discard reverted and reverter code, and merge intra fix changes

Then force push or should I make a new PR?

use the force, luke

Copy link
Collaborator

@fcartegnie fcartegnie left a comment

Choose a reason for hiding this comment

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

unsure why we need to commit so much code / project template

@fcartegnie fcartegnie merged commit f6d84a1 into fossasia:master Jun 6, 2024
@kienvo
Copy link
Member Author

kienvo commented Jun 6, 2024

I could write a script to automatically download and generate StdPeriphDriver if this is considered as the optimal way.

@simon-budig
Copy link
Collaborator

Please keep StdPeriphDriver in the repository. I believe it would be a bad idea to have our repository depend on Code pulled in at build-time from an external source. That creates a dependency we don't want.

(admittedly I don't like the StdPeriphDriver code. I believe at some point we might want to fix/modify it. It is inconsistent within itself and has some stupid design descisions. Being able to fix it is an important reason to keep the code in our repository.)

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.

None yet

3 participants