Add config options for Reset/Power Chip/Line

[jdeinum]

Preface

I really hope that its fine to open an issue for this. Otherwise I am happy to move this to the forum.

Possible Duplicate

This is similar to #15 , but I don't think the RAK 5146 has these changes yet based on commit d3b039f.

Feature

I am using a RAK 5146 on a custom built board running Debian 11, and am looking to migrate to concentratord. The only thing stopping me right now is that it seems like gpiochip0 is hard coded in the config for the RAK 5146 located in chirpstack-concentratord-sx1302/src/config/vendor/rak/rak5146.rs:

sx1302_reset_pin: Some((
    "/dev/gpiochip0".to_string(), // <----------------------------- gpiochip0 hardcoded here
    conf.gateway.sx1302_reset_pin.unwrap_or(17),
)),
sx1302_power_en_pin: conf
    .gateway
    .sx1302_power_en_pin
    .map(|v| ("/dev/gpiochip0".to_string(), v)), // <----------------------------- and here
..Default::default()

It would be great for flexibility to provide a gpiochip number and a gpio line for each possible pin in the config, but at minimum it would be nice just to specify a GPIO chip.

Can I do this myself

Probably yes, although I do want to know whether you want all of them to be consistent or whether I can just change the 5146. I haven't looked super in depth at the code but I imagine that changes will need to occur in:

1. The config as above

File: chirpstack-concentratord-sx1302/src/config/vendor/rak/rak5146.rs

sx1302_reset_pin: Some((
    conf
    .gateway
    .sx1302_reset_chip
    .clone()
    .unwrap_or("/dev/gpiochip0".to_string()),

    conf
    .gateway
    .sx1302_reset_line
    .unwrap_or(17),
)),

It would have to be changed for each vendor/model combo if we want to be consistent.

2. The config itself

File: chirpstack-concentratord-sx1302/src/config/mod.rs

Instead of just having a single pin for each utility, we'll specify both chip and line:

pub sx1302_reset_chip: Option<u32>,
pub sx1302_reset_line: Option<u32>,
pub sx1302_power_en_chip: Option<u32>,
pub sx1302_power_en_line: Option<u32>,
pub sx1261_reset_chip: Option<u32>,
pub sx1261_reset_line: Option<u32>,

Others

I am not fully sure how the toml is parsed in the project, so there might need to be changes to field names / macros somewhere else. I am also anticipating that I'll need to modify some tests. It's been a bit since I've worked with rust, so it'll take me some time to figure out what needs to change.

---

If you give me an idea of whether I am on the right track / missing anything, I'd be happy to write the code and make a PR. Thank you for all of the work you put into the stack!

[brocaar]

I would welcome a PR for this and I think it would be good to make everything consistent (also for the 2g4 and sx1301).

Some feedback:

To keep backwards compatible, would it be an idea to use the naming like sx1302_reset_pin_chip? Thus we will have the following pair:

• sx1302_reset_pin (Option<u32>)
• sx1302_reset_pin_chip (Option<string>)

To avoid all the duplication like:

sx1302_reset_pin: Some((
    conf
    .gateway
    .sx1302_reset_chip
    .clone()
    .unwrap_or("/dev/gpiochip0".to_string()),

    conf
    .gateway
    .sx1302_reset_line
    .unwrap_or(17),
)),

Maybe it would be better to create a get_sx1302_reset_pin(default_chip: &str, default_pin: u32) method, so that it can be used like:

sx1302_reset_pin: Some(conf.gateway.get_sx1302_reset_chip("/dev/gpiochip0", 17)),

Then the get_sx1302_reset_pin method will deal with the .unwrap_or(...). I think this makes the code a lot cleaner :-)

The sx1302_reset_pin is just an example, the above would apply to all XXX_pin settings.

[jdeinum]

That's a great idea. I wrote some simple methods and put it in the same file where Gateway is defined (chirpstack-concentratord-sx1302/src/config/mod.rs):

impl Gateway {
    pub fn get_sx1302_reset_pins(&self, default_chip: &str, default_pin: u32) -> Option<(String, u32)> {
        let chip = self.sx1302_reset_chip.clone().unwrap_or(default_chip.to_string());
        let pin = self.sx1302_reset_pin.unwrap_or(default_pin);
        info!("SX1302 Set Reset Chip={} and Reset Pin={:?}", chip, pin);
        Some((chip, pin))
    }

    pub fn get_sx1302_power_en_pins(&self, default_chip: &str, default_pin: u32) -> Option<(String, u32)> {
        let chip = self.sx1302_power_en_chip.clone().unwrap_or(default_chip.to_string());
        let pin = self.sx1302_power_en_pin.unwrap_or(default_pin);
        info!("SX1302 Set Power Enable Chip={} and Power Enable Pin={:?}", chip, pin);
        Some((chip, pin))
    }

    pub fn get_sx1261_reset_pins(&self, default_chip: &str, default_pin: u32) -> Option<(String, u32)> {
        let chip = self.sx1261_reset_chip.clone().unwrap_or(default_chip.to_string());
        let pin = self.sx1261_reset_pin.unwrap_or(default_pin);
        info!("SX1261 Set Reset Chip={} and Reset Pin={:?}", chip, pin);
        Some((chip, pin))
    }
}

I can't really think of a way to remove most of the redundancy, so I just kept it simple and separated it for these pins. There was a 4th pin described in one file but it wasn't used as far as I could tell.

All of the calls for pins in 1302 have been replaced with something along the lines of:

// ...
sx1302_reset_pin: conf.gateway.get_sx1302_reset_pins("/dev/gpiochip0", 17),
sx1302_power_en_pin: conf.gateway.get_sx1302_power_en_pins("/dev/gpiochip0", 18),

Let me know if this is along the lines of what you are looking for!

In regards to sx1301, I see there is only 1 pin described in the Config:

#[derive(Default, Serialize, Deserialize)]
pub struct Gateway {
    // ....
    pub reset_pin: Option<u32>,
}

Is there a more specific name you want for this pin? Or should I just leave it as is?

[jdeinum]

One other thing I noticed was that there was no default for the power enable pin for models rak2287 or rak5146. What do you recommend I set these values to? Or should I just set them to optional?

+++ b/chirpstack-concentratord-sx1302/src/config/vendor/rak/rak2287.rs
@@ -571,14 +571,10 @@ pub fn new(conf: &config::Configuration) -> Result<Configuration> {
-        sx1302_power_en_pin: conf
-            .gateway
-            .sx1302_power_en_pin
-            .map(|v| ("/dev/gpiochip0".to_string(), v)),                                   <------ v here is None if not specified in file I think?
+        sx1302_reset_pin: conf.gateway.get_sx1302_reset_pins("/dev/gpiochip0", 17),
+
+        // TODO: What should the default pin be? It was not specified in the original code
+        sx1302_power_en_pin: conf.gateway.get_sx1302_power_en_pins("/dev/gpiochip0", 18),  <---- random placeholder
         ..Default::default()
     })
 }

I do not have much experience with these boards and don't want to make any assumptions.

[brocaar]

These default to None, so I think you can just set these to None instead of using a default value. See also:

https://github.com/RAKWireless/rak_common_for_gateway/blob/master/lora/rak2287/reset_lgw.sh

[jdeinum]

I got a little sidetracked, and changed things slightly.

Instead of having similar methods exist in each of the chirpstack-concentratord-* , I created a new type under libconcentratord/src/pin_config.rs that holds all of the related pin info, including the methods to handle which pins / chips are chosen:

pub struct PinConfig {
    name: String,
    config_chip: Option<String>,
    default_chip: Option<String>,
    config_pin: Option<u32>,
    default_pin: Option<u32>,
}

impl PinConfig {
    fn choose_chip(&self) -> Option<&String> {
        match (self.config_chip, self.default_chip) {
            (Some(config_chip), _) => Some(&config_chip),
            (_, Some(default_chip)) => Some(&default_chip),
            (None, None) => None,
        }
    }

    fn choose_pin(&self) -> Option<u32> {
        match (self.config_pin, self.default_pin) {
            (Some(config_pin), _) => Some(config_pin),
            (_, Some(default_pin)) => Some(default_pin),
            (None, None) => None,
        }
    }

    pub fn get_pin_configuration(&self) -> Option<(&String, u32)> {
        match (self.choose_chip(), self.choose_pin()) {
            (Some(chip), Some(pin)) => Some((chip, pin)),
            (_, _) => None,
        }
    }
}

The code isn't fully worked through and has some errors at the moment, but that's the idea I have in mind. Then each of the chirpstack-concentratord-*/src/config/mod.rs vendors use the following code:

impl Gateway {
    pub fn get_sx1302_reset_pins(
        &self,
        default_chip: Option<&str>,
        default_pin: Option<u32>,
    ) -> Option<(String, u32)> {
        let pin_config = PinConfig {
            name: "sx1302_reset",
            config_chip: self.sx1302_reset_chip.clone(),
            default_chip,
            config_pin: self.sx1302_reset_pin,
            default_pin,
        };
        return pin_config.get_pin_configuration();
    }
// ...

what are your thoughts on something like this?

[brocaar]

To be honest, I think I prefer the previous implementation and I'm not sure if there is a big win with the above approach.

My brain digest this easier:

        let chip = self.sx1302_power_en_chip.clone().unwrap_or(default_chip.to_string());

Than this:

        match (self.config_chip, self.default_chip) {
            (Some(config_chip), _) => Some(&config_chip),
            (_, Some(default_chip)) => Some(&default_chip),
            (None, None) => None,
        }

My proposal would be to go with the initial approach. I left a few comments with regards to _pins -> _pin rename. Other than that, I think the original approach is fine to merge.

[brocaar]

This was implemented by https://github.com/chirpstack/chirpstack-concentratord/pull/126