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

Initial SubGhz development #170

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

QuantumEF
Copy link

Initial work, opening draft pull request to solicit feedback and comments.

This is not yet finished as I have not yet implemented the builder for the CustomSubGhzPreset to allow for custom presets.

@QuantumEF
Copy link
Author

With this latest commit, the following basic app does work and does not crash.

//! Template project for Flipper Zero.
//! This app prints "Hello, Rust!" to the console then exits.

#![no_main]
#![no_std]

// Required for panic handler
// use flipperzero_rt;

extern crate flipperzero_alloc;

use core::ffi::CStr;

use flipperzero::dialogs::alert;
use flipperzero::println;
use flipperzero::subghz;
use flipperzero_rt::{entry, manifest};

// Define the FAP Manifest for this application
manifest!(
    name = "Flipper Zero Rust",
    app_version = 1,
    has_icon = true,
    // See https://github.com/flipperzero-rs/flipperzero/blob/v0.11.0/docs/icons.md for icon format
    icon = "rustacean-10x10.icon",
);

// Define the entry function
entry!(main);

// Entry point
fn main(_args: Option<&CStr>) -> i32 {
    alert("Hello Erik");

    let dev_name = c"cc1101_int";

    let x = subghz::SubGhz::subghz_devices_get_by_name(dev_name);

    match x {
        Some(_) => alert("YEA"),
        None => alert("Boooo"),
    }

    0
}

@QuantumEF
Copy link
Author

@dcoles Do you have time to give feedback on what I have done so far? ie: just how I have started going about doing things.
As mentioned in my previous comment, I have only tested just opening device. I am continuing testing to validate the functionality of other things.

@JarvisCraft JarvisCraft self-requested a review October 12, 2024 22:08
Copy link
Contributor

@JarvisCraft JarvisCraft left a comment

Choose a reason for hiding this comment

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

The first iteration of the changes

They maybe nitpicky, but I wish that they will be helpful

@@ -41,6 +41,7 @@ embedded-hal-0 = { package = "embedded-hal", version = "0.2.7", features = ["unp

# Docs
document-features = { workspace = true, optional = true }
uom = { version = "0.36.0", features = ["si", "u32"], default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that we add all new dependencies to workspace root and reference them here via workspace = true

Also note, that this part of Cargo.toml is for docs-related dependencies (see comment two lines above)

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense to me, I will hold off on this change for now since I need to properly figure out how to use uom crate since at present my use of it utilizes dynamic dispatch. I also want to read up on cargo workspaces since I have not used them before.

crates/flipperzero/src/subghz/device.rs Outdated Show resolved Hide resolved
crates/flipperzero/src/subghz/device.rs Outdated Show resolved Hide resolved
crates/flipperzero/src/subghz/device.rs Show resolved Hide resolved
crates/flipperzero/src/subghz/device.rs Outdated Show resolved Hide resolved

impl<'a> CustomSubGhzPreset<'a> {
/// # Safety
/// Read the manual for the CC1101 chip and review the [`furi_hal_subghz_load_custom_preset()`](https://github.com/flipperdevices/flipperzero-firmware/blob/0eaad8bf64f01a6f932647a9cda5475dd9ea1524/targets/f7/furi_hal/furi_hal_subghz.c#L162) function provided by the flipper API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Read the manual for the CC1101 chip and review the [`furi_hal_subghz_load_custom_preset()`](https://github.com/flipperdevices/flipperzero-firmware/blob/0eaad8bf64f01a6f932647a9cda5475dd9ea1524/targets/f7/furi_hal/furi_hal_subghz.c#L162) function provided by the flipper API
/// Read the manual for the CC1101 chip and review the [`furi_hal_subghz_load_custom_preset()`].
///
/// [`furi_hal_subghz_load_custom_preset()`]: (https://github.com/flipperdevices/flipperzero-firmware/blob/0eaad8bf64f01a6f932647a9cda5475dd9ea1524/targets/f7/furi_hal/furi_hal_subghz.c#L162) function provided by the flipper API

Copy link
Author

Choose a reason for hiding this comment

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

Not quite sure what exactly the change is here. My assumption is for it to include both a link to the function in the flipperzero_sys::furi_hal_etc documentation as well as the link I provide to the C source code?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I've accidentally trimmed the end of the comment sentence.

The intention here is to move the URL to the markdown footer so that the header text is easier to read

}

impl<'a> SubGhzPreset<'a> {
pub fn into_furi_preset(&self) -> FuriHalSubGhzPreset {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but maybe its worth adding impl From<&SubGhzPreset<'_>> for FuriHalSubGhzPreset>

Also, as_ prefix seems to be more valid for the function

Copy link
Author

Choose a reason for hiding this comment

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

I am probably going to hold off for now on implementing this since the docs mention that "The From trait is intended for perfect conversions," This function looses data when converting from the Custom variant.

I like the as_ suggestion because it is something I always forget the semantics about (I should print it out, lol).
When I originally created some of the structs, I was not able to implement copy, but now I am since the custom variant is Custom(&[u8]). Because of this, I am wondering if I should just change the function signature to take in self instead of a &self then maybe implying is should use to_ ?

crates/flipperzero/src/subghz/preset.rs Outdated Show resolved Hide resolved
crates/flipperzero/src/subghz/preset.rs Outdated Show resolved Hide resolved
crates/flipperzero/src/subghz/error.rs Outdated Show resolved Hide resolved
@QuantumEF
Copy link
Author

The first iteration of the changes

They maybe nitpicky, but I wish that they will be helpful

Thank you so much for your feedback and help! As you see I have gone through most of your suggestions and added them. The ones I have not marked as resolved yet are because I did want to think on them and possible discuss them more.

In my error, I lost some of the comments on the changes I marked as resolved because I assumed the "Marked as resolved" button did something similar to the "Comment and mark as resolved" button I have seen elsewhere; Oops, most of those comments were just referencing the commit I changed things in.

@JarvisCraft
Copy link
Contributor

Thank you so much for your feedback and help!

You are welcome! Thank you for working on this feature!

As you see I have gone through most of your suggestions and added them. The ones I have not marked as resolved yet are because I did want to think on them and possible discuss them more.

For sure! I understand that this is a draft, so my review should be considered as more of the recommendations by now :)

In my error, I lost some of the comments on the changes I marked as resolved because I assumed the "Marked as resolved" button did something similar to the "Comment and mark as resolved" button I have seen elsewhere; Oops, most of those comments were just referencing the commit I changed things in.

No problems, GitHub is a bit weird with PR review threads anyway

@JarvisCraft
Copy link
Contributor

It may take me some time to continue the review (I will be at the conference for a few days) thus feel free to leave some questions unresolved as-is since eventually the will be reviewed

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