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

orb-ui: show car simulation #223

Closed
wants to merge 52 commits into from
Closed

orb-ui: show car simulation #223

wants to merge 52 commits into from

Conversation

fouge
Copy link
Collaborator

@fouge fouge commented Sep 17, 2024

No description provided.

@fouge fouge requested a review from dangirsh September 17, 2024 16:33
@fouge fouge changed the title orb-ui: show car orb-ui: show car simulation Sep 17, 2024
@fouge fouge force-pushed the fouge/show-car branch 3 times, most recently from b3e6919 to 61ae502 Compare September 23, 2024 15:57
tweaked colors
new event for biometric capture start
new scenario: self-serve or operator-assisted
based on reference designs.
simulation with error during capture
to sync them: start progress only when shroud stops breathing.
delay can now be set before playing a sound.
getting ready for show-car
wait for button press
normalize brightness at top and bottom of ring
fixes ORBP-146
grouped the LED by 2 so that colors are more visible, less blended.
better debug experience
better transitions
idle state with static color
reset gimbal value to None once sent
because we don't use a stack, we need to
store the current state and return if
the animation is in ForceStop state.
renamed previous SignupStart to SignupStartOperator
use SignupStart like done before everywhere else.
and remove useless code: self_serve flag
in case capture takes time, progress ring is restarted from scratch
@fouge fouge changed the base branch from fouge/orb-ui-self-serve to main September 26, 2024 16:54
.gitattributes Outdated
@@ -1 +1,77 @@
*.wav filter=lfs diff=lfs merge=lfs -text
orb-ui/sound/assets/sound_user_start_capture.wav filter=lfs diff=lfs merge=lfs -text
Copy link
Collaborator

Choose a reason for hiding this comment

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

You already filter *.wav. Are these actually necessary?

@@ -50,6 +52,7 @@ struct Sound {
id: u64,
reader: Box<dyn Reader>,
name: String,
start_time: SystemTime,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually use Instant since it is monotonic.

@@ -42,6 +42,8 @@ pub struct Alert<const N: usize> {
phase: f64,
/// first edge from pattern\[0\] to pattern\[1\] has LEDs on
active_at_start: bool,
/// initial delay, in seconds, before starting the animation
initial_delay: f64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually we should always use std::time::Duration. Is there a reason you chose a f64 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dt is f64 so it's very simple to use it like this

@@ -63,6 +63,10 @@ impl<const N: usize> ArcDash<N> {
impl<const N: usize> Animation for ArcDash<N> {
type Frame = RingFrame<N>;

fn name(&self) -> &'static str {
"ArcDash"
Copy link
Collaborator

@TheButlah TheButlah Sep 26, 2024

Choose a reason for hiding this comment

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

Suggested change
"ArcDash"
std::any::type_name::<Self>()

Try doing it this way in the default trait impl to remove the repetition. If that doesn't work, then just do it on each struct like you already do.

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 did it like this

@@ -60,9 +64,12 @@ impl<const N: usize> Animation for ArcPulse<N> {
AnimationState::Running
}

fn transition_from(&mut self, superseded: &dyn Any) {
fn transition_from(&mut self, superseded: &dyn Any) -> eyre::Result<bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is superseded a dyn Any? Why is failure to transition a thing at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's now returning the bool only

transition_time: f64,
}

struct MilkyWayConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need comments to explain what each one of these numbers are.

#[allow(dead_code)]
#[must_use]
pub fn new(background: Option<Argb>) -> Self {
let mut config = MILKY_WAY_DEFAULT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

strange to have a config but not actually make it configurable.

I suggest:

#[derive(Debug, Eq, PartialEq, Clone)]
struct MilkyWayConfig  {
    pub background: Argb,
    pub fade_delta: i8,
    pub red_delta: i8,
    pub green_delta: i8,
    pub blue_delta: i8,
    pub red_min_max: (u8, u8),
    pub green_min_max: (u8, u8),
    pub blue_min_max: (u8, u8),
}

// Refer to things with named presets instead of "default". Fewer implicit assumptions down the line.
impl MilkyWayConfig {
    fn descriptive_name_for_preset() -> Self {
        Self { ... } // fill in values for the preset
    }

    fn another_preset() -> Self {
        Self { .. } // fill values
    }
}


impl MilkyWay {
    pub fn new(config: MilkyWayConfig) { ... }
}

Here's how you use it:

// Use struct update syntax to only override some of the fields of the preset
MilkyWay::new(MilkyWayConfig { background: Argb::red(), fade_delta: 2, ..MilkyWayConfig::descriptive_name_for_preset()})
// Use the preset without customization.
MilkyWay::new(MilkyWayConfig::descriptive_name_for_preset())
// Use a different preset
MilkyWay::new(MilkyWayConfig::another_preset())

}
}

#[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use expect and not allow

@@ -63,6 +63,10 @@ impl<const N: usize> ArcDash<N> {
impl<const N: usize> Animation for ArcDash<N> {
type Frame = RingFrame<N>;

fn name(&self) -> &'static str {
Copy link
Collaborator

@TheButlah TheButlah Sep 26, 2024

Choose a reason for hiding this comment

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

It is possible to make this a const NAME: &str; in the trait instead of a method? Asking if possible because im not sure if the const will work with trait objects.

@@ -63,6 +63,10 @@ impl<const N: usize> ArcDash<N> {
impl<const N: usize> Animation for ArcDash<N> {
type Frame = RingFrame<N>;

fn name(&self) -> &'static str {
Copy link
Collaborator

@TheButlah TheButlah Sep 26, 2024

Choose a reason for hiding this comment

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

Instead of using a name method, why not use a kind method that returns a enum AnimationKind { ArcDash, MilkyWay, ... } that identifies which animation it is?

That enum can then just be printed with Debug.

@fouge fouge closed this Oct 2, 2024
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