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

fix: close relays at startup after we load config #526

Closed
wants to merge 1 commit into from

Conversation

mtfurlan
Copy link
Contributor

@mtfurlan mtfurlan commented Jul 8, 2022

If you aren't using OFFICIALBOARD and your relay isn't active high on
pin 13 it will be left open on startup.

I feel like this is inconsistant because I thought it was working a few days ago
but all the commits I might have had running don't lock my test door controller,
so I dunno.

If you aren't using OFFICIALBOARD and your relay isn't active high on
pin 13 it will be left open on startup.
@matjack1
Copy link
Collaborator

matjack1 commented Jul 8, 2022

isn't this happening already here: https://github.com/esprfid/esp-rfid/blob/dev/src/config.esp#L166 ?

@mtfurlan
Copy link
Contributor Author

mtfurlan commented Jul 8, 2022

Oh interesting.
It looks like my problem is that I have the official board so I am compiling with -DOFFICIALBOARD, but my relay needs to be asserted high to lock the door.
So the bug is just that if your lock is active high on the official board it doesn't lock.
The inconsistency I saw is probably that I was compiling with and without OFFICIALBOARD at different times.

Is there any reason that shouldn't happen there regardless of if it is an OFFICIALBOARD?

What is the reasoning behind the OFFICIALBOARD stuff?
I had expected that it just defaults some things like which pin is the relay pin, but it appears to do a lot more.

@matjack1
Copy link
Collaborator

matjack1 commented Jul 8, 2022

I'm not 100% sure how the official board works unfortunately, but it's a custom PCB with specific components. So if you have the official board you don't need to setup anything, it's all ready with zero configuration. If you have any other compatible hardware you shouldn't use the OFFICIALBOARD flag. Makes sense?

@mtfurlan
Copy link
Contributor Author

mtfurlan commented Jul 8, 2022

I have the PCB in the images in the readme from the tindie store.
I set the relay to "active low".
If I compile with -DOFFICIALBOARD then it doesn't lock on boot, if I don't compile with it it does lock on boot.

We could remove the #ifndef OFFICIALBOARD around the code you linked and it would boot up as I expected, though I feel like I'm missing something about how OFFICIALBOARD is supposed to be used.
It assumes stuff like having a wiegand reader connected, so maybe OFFICIALBOARD is supposed to be the PCB with a specific set of stuff connected that works in a specific way, such as an active high relay and a wiegand reader and in that case we should leave it alone?

Either way I think I'm going to not compile with -DOFFICIALBOARD again so it doesn't ignore configuration stuff.

@matjack1
Copy link
Collaborator

Now that I'm checking the board, I can see that the old version supports only the Wiegand and the new one also other readers.

I'm not sure if @nardev is reading us, but probably we can drop the OFFICIALBOARD code with the new board, as it's customizable and it doesn't make sense anymore. @omersiar what do you think? It would make the code much nicer and easier! :)

@nardev
Copy link
Collaborator

nardev commented Jul 13, 2022

@matjack1 you are right!

@matjack1
Copy link
Collaborator

Thank you @nardev for confirming! I might try getting rid of the OFFICIALBOARD flag and simplify everything a bit then ;)

@nardev
Copy link
Collaborator

nardev commented Jul 15, 2022 via email

@matjack1
Copy link
Collaborator

It's good to know that you are still using the project!

We could think about shipping some configs that you can pick from the UI maybe? So you can pick the settings the first time? When I'll play around to remove the build flag I'll try something out and let you know!

@nardev
Copy link
Collaborator

nardev commented Jul 15, 2022 via email

@matjack1
Copy link
Collaborator

matjack1 commented Nov 6, 2022

I've added this issue: #545 to track what's still needed.

I'm going to close this PR as this should not be needed anymore, OK?

Let me know if I'm wrong though :)

@matjack1 matjack1 closed this Nov 6, 2022
@mtfurlan mtfurlan deleted the fix/lock-door-at-boot branch November 7, 2022 16:43
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.

3 participants