add wiegand module #3203

ccutrer · 2020-07-09T01:00:34Z

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/*.

Add a module to support Wiegand keycard readers. I first tried using the gpio module, but quickly realized that doing it in Lua was too slow to properly capture the protocol. All the bits got through, but because there are multiple data lines, and they were fast, they got lumped together and you couldn't fix the original order of the bits

pjsg · 2020-07-09T01:17:44Z

app/modules/wiegand.c

+    luaL_unref(L, LUA_REGISTRYINDEX, wiegand_cb_ref);
+    wiegand_cb_ref = LUA_NOREF;
+  }
+  return 0;


I suspect that you ought to release the interrupt hook here.

I was trying to figure out how to do that. AFAICT, the softuart module doesn't ever unregister its hook. The rotary module does, but using a lower level function. I can't figure out the the gpio module explicitly registers their hook - it seems it's just uses an even lower level function to create a fallback hook. Any tips on the recommended way to do it?

If there isn't an easy way to do this, just make sure that nothing bad happens if the interrupt hook is called when the device is closed. Also, if the device is reopened, then don't reregister the hook (or maybe the hook registration code already takes care of it). Please add a comment to say what strategy you are following in the close function.

pjsg · 2020-07-09T01:24:49Z

I think that you probably want to handle the timer somewhat differently -- there can be significant delays between posting the task and when it gets to run. You might want to save the value of system_get_time in the interrupt handlers and then in the task set the timer for the appropriate time (the timeout + last bit time - current time).

Also, try not to post tasks if the previous one has not yet been scheduled. The queues are not very large and posting 30 tasks will certainly overflow the queue -- and could cause other tasks to be dropped as well.

ccutrer · 2020-07-09T01:39:50Z

I'll definitely adjust the task posting. In terms of the timeout, I wasn't really concerned about being precise. It just needs to not try to collect the message until at least that long as passed. I wanted to avoid dealing with clock wraparounds because I know they can be pitfalls. The amount of time between individual bits in a message (< 20ms) is microscopic compared to the amount of time between separate messages (I can't get my keypad to send faster than 1 message/s when I'm mashing it, and with an actual RFID card it's nearly 2s.

marcelstoer · 2020-07-09T10:53:35Z

tools/luacheck_config.lua

+        setup = empty,
+        close = empty
+      }
+    }


Not sure if you noticed but the CI build failed.

Critical error: Couldn't load configuration from tools/luacheck_config.lua: syntax error (line 726: '}' expected (to close '{' at line 5) near 'wifi')

nwf · 2020-07-11T11:37:35Z

Ah, the Wiegand protocol. Thank you for taking a stab at doing it in Lua, but yea, this one is very timing sensitive and probably needs to be in C.

However, be advised that the progenitor library, https://github.com/monkeyboard/Wiegand-Protocol-Library-for-Arduino, says in its README that

This library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public License

I have no issue with (L)GPL (in fact, much prefer it for my own work), but the NodeMCU tree is mostly MIT. We should perhaps have a mechanism to permit clearly documented exceptions that may change the distribution requirements for the resulting binaries (in this case, by adding an obligation to provide the wiegand module source).

ccutrer · 2020-07-13T15:41:32Z

Ah, the Wiegand protocol. Thank you for taking a stab at doing it in Lua, but yea, this one is very timing sensitive and probably needs to be in C.

However, be advised that the progenitor library, https://github.com/monkeyboard/Wiegand-Protocol-Library-for-Arduino, says in its README that

This library is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public License

I have no issue with (L)GPL (in fact, much prefer it for my own work), but the NodeMCU tree is mostly MIT. We should perhaps have a mechanism to permit clearly documented exceptions that may change the distribution requirements for the resulting binaries (in this case, by adding an obligation to provide the wiegand module source).

Good point. I copied the license lines into the top of my wiegand.c but I must have only been skimming because in my mind I read it as MIT :). Do you think calling it out in README.md is sufficient, or should I also call out the exception in LICENSE?

nwf · 2020-07-13T17:01:27Z

If you don't mind, I'd like to mark this PR as blocked and raise a separate issue for the governance question?

ccutrer · 2020-07-13T17:09:32Z

The more I think about it, the more I believe I should probably just start over from scratch. The module gets statically linked in, so technically even the LGPL would require infecting the rest of the code base with the license.

marcelstoer · 2020-07-13T17:58:06Z

Good point. I copied the license lines into the top of my wiegand.c but I must have only been skimming because in my mind I read it as MIT :). Do you think calling it out in README.md is sufficient, or should I also call out the exception in LICENSE?

That's not enough unfortunately. Unless the source can be at least dual-licensed (which wouldn't make much sense in this case) we can't accept that, sorry. I remember we have had two similar cases in the past. In one case the original author was willing to change the license of his project to something compatible with MIT. In the other case the developer contributing to NodeMCU was determined to add the new module and rewrote it "without" the source of original lib to stay clear of licensing issues.

ccutrer · 2020-07-13T23:26:26Z

ok, PR updated. the Wiegand/gpio code has been completely re-written from scratch, so no more (L)GPL code. This means losing support for things besides 4 bit and 26 bit (the two types the keypad I have can send). I refactored things while I was doing it so that multiple instances can exist at the same time. While doing this, I figured out how to properly de-register my interrupt hooks (just "re-register" with a mask of 0; when registering for multiple instances it has to register a single hook for all pins at once). I also tweaked the timer code while I was in there to only delay the remaining time to get to 25ms in case the posted task takes a while to execute. Oh yeah, and another task isn't posted until the prior one runs. Phew! I learned quite a bit about the interrupt system and dbg_print while I was getting this one to work :).

nwf

Sorry for not reviewing sooner. I've one nit and some questions arising from ignorance, but this looks like it could land.

nwf · 2020-08-20T22:34:43Z

app/modules/wiegand.c

+      continue;
+    }
+
+  	++w->bit_count;


Whitespace nit

nwf · 2020-08-20T22:35:24Z

app/modules/wiegand.c

+    if ((gpio_bits & 1) == 0)
+      continue;
+    // find the struct registered for this pin
+    volatile wiegand_t w = pins_to_wiegand_state[i];


I don't believe there's need for this to be volatile?

pins_to_wiegand_state is volatile, so to assign it to another variable (it's a pointer) you'd have to cast the volatile away.

I'm familiar with the C type system, yes. I mean more "what is volatile accomplishing here"? Is the interrupt racing some mainline code such that the compiler using a cached value rather than rereading wouldn't work out? (Presumably the interrupt can't be using volatile to defend against concurrent mutation by mainline code?)

ah, okay. I'm not an expert at volatile and interrupts. assuming the compiler would only cache within the interrupt routine, we'd be fine. but if it caches across multiple calls to the interrupt, we would not - the same array can be updated by mainline code if you instantiate multiple instances of the wiegand object (to handle multiple card readers)

Yes, all value reuse is within routines like that. The C abstract machine (a PDP-11, essentially!) is single-threaded, which is why the compiler can cache memory values in registers: if we don't change it, surely it's not changed. This is also the origin of the "strict aliasing" rules: how do we know that a pointer doesn't alias the pointer we used to read a value? (We rely on the C type system, of all things, to answer that question.)

nwf · 2020-08-20T22:35:41Z

app/modules/wiegand.c

+{
+  lua_State *L = lua_getstate();
+
+  volatile wiegand_t w = (wiegand_t) param;


Similarly, might not need to be volatile.

I think you're right on this one.

app/modules/wiegand.c

* fix a whitespace error (tabs!!!!) * remove an unnecessary volatile qualifier

nwf · 2020-08-22T17:30:46Z

app/modules/wiegand.c

+    w->last_bit_time = system_get_time();
+
+    if (!w->task_posted) {
+      task_post_medium(tasknumber, (os_param_t)w);


Hm; forgive another naive question: what happens if you task_post the same task twice? If nothing bad (that is, if they don't queue up but instead posting is idempotent) I think you could replace this with if (!w->timer_running) ?

marcelstoer · 2020-10-18T18:45:02Z

@nwf are you ready to merge this?

nwf · 2020-10-19T00:04:32Z

Sure; let's get it some hopefully wider testing. I, sadly, lack the hardware to do so.

* add wiegand module * minor tweaks to wiegand module * fix a whitespace error (tabs!!!!) * remove an unnecessary volatile qualifier

pjsg reviewed Jul 9, 2020

View reviewed changes

marcelstoer added the new module label Jul 9, 2020

marcelstoer requested changes Jul 9, 2020

View reviewed changes

ccutrer force-pushed the wiegand branch from 85ba94d to 578d431 Compare July 13, 2020 23:22

ccutrer force-pushed the wiegand branch 2 times, most recently from f18752c to a556a68 Compare July 13, 2020 23:30

add wiegand module

e5593fa

ccutrer force-pushed the wiegand branch from a556a68 to e5593fa Compare July 14, 2020 22:25

marcelstoer approved these changes Aug 20, 2020

View reviewed changes

nwf reviewed Aug 20, 2020

View reviewed changes

minor tweaks to wiegand module

11fa963

* fix a whitespace error (tabs!!!!) * remove an unnecessary volatile qualifier

nwf reviewed Aug 22, 2020

View reviewed changes

marcelstoer added this to the Next release milestone Sep 13, 2020

nwf merged commit 63e1fcd into nodemcu:dev Oct 19, 2020

marcelstoer pushed a commit that referenced this pull request Nov 7, 2020

add wiegand module (#3203)

5ee658e

* add wiegand module * minor tweaks to wiegand module * fix a whitespace error (tabs!!!!) * remove an unnecessary volatile qualifier

vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Nov 21, 2020

add wiegand module (nodemcu#3203)

79c759b

* add wiegand module * minor tweaks to wiegand module * fix a whitespace error (tabs!!!!) * remove an unnecessary volatile qualifier

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add wiegand module #3203

add wiegand module #3203

ccutrer commented Jul 9, 2020 •

edited

Loading

pjsg Jul 9, 2020

ccutrer Jul 9, 2020

pjsg Jul 11, 2020

pjsg commented Jul 9, 2020 •

edited

Loading

ccutrer commented Jul 9, 2020

marcelstoer Jul 9, 2020

nwf commented Jul 11, 2020

ccutrer commented Jul 13, 2020

nwf commented Jul 13, 2020

ccutrer commented Jul 13, 2020

marcelstoer commented Jul 13, 2020 •

edited

Loading

ccutrer commented Jul 13, 2020

nwf left a comment

nwf Aug 20, 2020

nwf Aug 20, 2020

ccutrer Aug 21, 2020

nwf Aug 21, 2020

ccutrer Aug 21, 2020

nwf Aug 22, 2020

nwf Aug 20, 2020

ccutrer Aug 21, 2020

nwf Aug 22, 2020

marcelstoer commented Oct 18, 2020

nwf commented Oct 19, 2020

add wiegand module #3203

add wiegand module #3203

Conversation

ccutrer commented Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjsg commented Jul 9, 2020 • edited Loading

ccutrer commented Jul 9, 2020

Choose a reason for hiding this comment

nwf commented Jul 11, 2020

ccutrer commented Jul 13, 2020

nwf commented Jul 13, 2020

ccutrer commented Jul 13, 2020

marcelstoer commented Jul 13, 2020 • edited Loading

ccutrer commented Jul 13, 2020

nwf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelstoer commented Oct 18, 2020

nwf commented Oct 19, 2020

ccutrer commented Jul 9, 2020 •

edited

Loading

pjsg commented Jul 9, 2020 •

edited

Loading

marcelstoer commented Jul 13, 2020 •

edited

Loading