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.

Sign up for GitHub

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

Jump to bottom

Userspace LED Matrix Driver #49

Closed
wants to merge 7 commits into from
Closed

Userspace LED Matrix Driver #49

wants to merge 7 commits into from

Conversation

echavemann
Copy link
Collaborator

Userspace only, as requested. Had to add a couple syscalls but it creates a nice little LED wave

@echavemann echavemann self-assigned this Dec 8, 2024
@echavemann echavemann force-pushed the echavemann/userlib-led-matrix branch from 4f4772e to 9e53e2b Compare December 8, 2024 04:31
@echavemann echavemann force-pushed the echavemann/userlib-led-matrix branch from b18dfac to b510042 Compare December 8, 2024 04:37
@echavemann echavemann changed the base branch from main to echavemann/led-matrix-redo December 8, 2024 04:38
@echavemann echavemann force-pushed the echavemann/userlib-led-matrix branch from 44464b7 to a9afe92 Compare December 8, 2024 18:33
@echavemann echavemann force-pushed the echavemann/userlib-led-matrix branch from be852ff to 023b00b Compare December 8, 2024 19:29
stevenewald
stevenewald requested changes Dec 8, 2024
View reviewed changes
include/drivers/driver_enums.hpp Outdated
@@ -9,6 +9,9 @@ enum class DriverCommand {
BUTTONS = 2,
TERMINAL_OUTPUT = 3,
TIMER_CANCEL = 4,
SET_PIN = 5,
CLEAR_PIN = 6,
SET_PIN_OUTPUT = 7,
Copy link
Owner

Choose a reason for hiding this comment

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

set_pin and set_pin_output are kinda confusing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe make_pin_output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cahnging to config_pin_output

include/scheduler/pending_process_callbacks.hpp
@@ -11,7 +12,7 @@ namespace edge {
// TLDR: singleton vs everything owned by scheduler, food for thought
class PendingProcessCallbacks {
public:
static constexpr uint8_t MAX_READY_CALLBACKS = 10;
static constexpr uint8_t MAX_READY_CALLBACKS = 20;
Copy link
Owner

Choose a reason for hiding this comment

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

should we bump this to like 64? I can see a world where a lot could reasonably build up in one quantum

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree here but not really the actual issue

include/userlib/syscalls.hpp Outdated

void clear_pin(uint32_t pin);

void make_pin_output(uint32_t pin);
Copy link
Owner

Choose a reason for hiding this comment

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

nit: make names more clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/drivers/driver_commands.cpp
@@ -30,6 +32,15 @@ etl::optional<int> handle_command(DriverCommand type, int arg1, int arg2, int ar
static_cast<uint32_t>(arg1)
);
break;
case DriverCommand::SET_PIN:
edge::aidan::set_gpio_pin(static_cast<uint32_t>(arg1));
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather have an actual driver for GPIO vs driver_commands directly interacting with the HAL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think this is fine for the illustrative purpose

src/drivers/driver_commands.cpp Outdated
edge::aidan::clear_gpio_pin(static_cast<uint32_t>(arg1));
break;
case DriverCommand::SET_PIN_OUTPUT:
GPIOPin(static_cast<uint32_t>(arg1), GPIOConfiguration::OUT);
Copy link
Owner

Choose a reason for hiding this comment

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

This feels like a really weird way to do this...

Side note (I'm willing to do this), it could be cool to have GPIOPin be a userspace object that is returned by a syscall

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could but doesnt seem needed and i dont love expoisng gpio to userspace if we want to build abstraction, cc these syscalls really just being here for illustration

src/scheduler/pending_process_callbacks.cpp Outdated
@@ -1,4 +1,5 @@
#include "scheduler/pending_process_callbacks.hpp"
#include "userlib/syscalls.hpp"
Copy link
Owner

Choose a reason for hiding this comment

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

Revert?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/user_programs/user_program_led_matrix.cpp Outdated
Comment on lines 1 to 3
#include "hal/gpio_wrapper.hpp"
#include "microbit_v2.h"
#include "scheduler/pending_process_callbacks.hpp"
Copy link
Owner

Choose a reason for hiding this comment

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

Revert 1 and 3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/user_programs/user_program_led_matrix.cpp Outdated

void upkeep_led_matrix(uint32_t id)
{
edge::userlib::clear_pin(rows[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

for_each

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, other usages too

src/user_programs/user_program_led_matrix.cpp Outdated

#include <cstdio>

static uint8_t row_index = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Can some of these be non-static (moved into stack on led_matrix_task) or at least static function variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all on stack now

@stevenewald
Copy link
Owner

Happy to handle some of these since some are non critical to getting the demo ready, although none should take very long. lmk

Base automatically changed from echavemann/led-matrix-redo to main December 10, 2024 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenewald stevenewald stevenewald requested changes

Assignees

@echavemann echavemann

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@echavemann @stevenewald