margyle / decaf

Decaf: Does Every Coffee Action, Friend
26 stars 10 forks source link

relayControl JSON output does not have resource names #2

Closed margyle closed 5 years ago

margyle commented 5 years ago

Current output:

{
  "relayControl": {
    "relayController": [
      "23",
      "1",
      "2",
      "0",
      "grinder"
    ]
  }
}
jjok commented 5 years ago

Hey. I was about to open an issue suggesting that you send all the parameters for the relay control in the body of the message, as JSON or something, but that might be what this issue relates to.

{
  "pinNumber" : 23,
  "relayChannel": 1,
  "timeOn": 2,
  "repeatValue": 2,
  "repeatDelay": 4,
  "connectedHardware": "grinder"
}

And post it to http://192.168.1.183:5000/relayControl

joelhaasnoot commented 5 years ago

Or at minimum have a way to do this with query parameters... http://192.168.1.183:5000/relayControl?relayChannel=1&timeOn=2&repeatValue=2&repeatDelay=4&connectedHardware=grinder.

One thing that has me puzzled though, why do you need the name of the hardware in this case? It would make sense for the API to either a) know which device is on which pin or b) totally not care about the devices and go with pins. Or is it used for logging?

margyle commented 5 years ago

Totally agree Re: query parameters.

As for the hardware name, that's just the first parameter I added to the function. You will be able to go by pin number as well. I also did this piece before I did the pinInfo function. My intention with starting with hardware name/type was to simplify brew control for people that are not as experienced as you and I might be. While it's a no brainer for us to use pin addresses, "turn on grinder" makes more sense for folks who are just learning about coding or pi's in general.

So, I totally agree with you on that as well, just haven't implemented it yet.

jjok commented 5 years ago

@margyle Is this a generic endpoint that can do things other than the grinder?

margyle commented 5 years ago

@jjok Yes, Mugsy's relay board has 4 channels. This is meant to control any of those channels(minus the heater as discussed in the Locking issue discussions). There will be a separate endpoint that is connection agnostic and can control any pin.

jjok commented 5 years ago

I see. I wonder if it would be better to have explict endpoints for each thing, rather than a generic one. That way if would be easier to understand what functionality is offered by the API. It should also make each one a little simpler, as you presumably wouldn't need to pass the pin number and relay channel.

 {
  "timeOn": 2,
  "repeatValue": 2,
  "repeatDelay": 4
}

And post it to /grinder, which we already know is pin 23 and relay channel 1 (do we?).

That's how I'd prefer to do it; make everything super-explicit, rather than super-generic.

Maybe? 🤔

margyle commented 5 years ago

I think that is a good point. It would simplify development for stock Mugsy setups. There could then be an endpoint that is more generic for those people that add their own hardware to the setup.

And yeah, the pinMappings table is meant to inform the app/user on what is connected to a pin.

jjok commented 5 years ago

for those people that add their own hardware to the setup

@margyle RE: Licenses, which I'll open as a separate issue, we should probably think about how a Copyleft license affects people who want to make their own customisations. eg Me, in my kitchen. Should I be required to open-source my code? Have you thought about that? Should users/developers be able to add their own plugins without open-sourcing them?

margyle commented 5 years ago

@jjok There are lot of details to sort out there. Maybe a dual license? Like if it's running on an actual Mugsy it's MIT but on a different coffee maker its some flavor of GPL? I'm heading out to a meeting but feel free to open up that issue and we can start sorting it out.

levi commented 5 years ago

Hi all,

I have a branch up on my fork that resolves the return value and also moves the two action methods to be POST requests -- https://github.com/levi/decaf/pull/1. It builds upon #18, so I am unable to open it up as a pull request until some version of that PR is landed. Let me know your thoughts on the changes (feel free to comment inline on the PR).

Cheers!

margyle commented 5 years ago

@levi Thanks! I'll be getting to these merges this evening. Very happy to have you helping out!