-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add RFID support and upgrade OCPP library #325
Conversation
Voltage is used by divert mode, as well as sent along to the OpenEVSE module for power calculations. Setting this to zero could result in a lot of issues. The divert code would see divide by zero happen, and the energy montoring code on the OpenEVSE module would result in the recorded energy usage being incorrect. While this shouldn't happen reguarly, I found myself in this situation by sending a null/zero-length message to the relevant topic that the ESP32 was listening on. Because there is no error checking in the float parsing code, a null value parsed to zero, which resulted in the voltage value being set to zero. An alternative here would be to reset the voltage to `DEFAULT_VOLTAGE` if a zero or invalid value was received. As written, this will simply do nothing and ignore the invalid/zero value.
A value between 60 and 300 volts, while being a generous range, should prevent issues where the value read from MQTT is read as a `0` or erroneously specified in millivolts.
…to rfid_and_timer
Jeremypoulter/issue5
Voltage is used by divert mode, as well as sent along to the OpenEVSE module for power calculations. Setting this to zero could result in a lot of issues. The divert code would see divide by zero happen, and the energy montoring code on the OpenEVSE module would result in the recorded energy usage being incorrect. While this shouldn't happen reguarly, I found myself in this situation by sending a null/zero-length message to the relevant topic that the ESP32 was listening on. Because there is no error checking in the float parsing code, a null value parsed to zero, which resulted in the voltage value being set to zero. An alternative here would be to reset the voltage to `DEFAULT_VOLTAGE` if a zero or invalid value was received. As written, this will simply do nothing and ignore the invalid/zero value.
A value between 60 and 300 volts, while being a generous range, should prevent issues where the value read from MQTT is read as a `0` or erroneously specified in millivolts.
I have tested the RFID support (local authorization) with great results. I was able to scan and save 25 tags and start charge sessions with tags. I used an Adafruit RFID reader https://www.adafruit.com/product/364. A custom OpenEVSE RFID reader will be tested and avaliable soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comment, overall looks good, however would be good to do lots of smaller PRs, one for each issue, rater than a single large PR, a lot easer to review.
src/web_server.cpp
Outdated
rfid.waitForTag(60); | ||
} | ||
|
||
void handlePollRFID(MongooseHttpServerRequest *request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not poll, better to add the data to the status endpoint and also send them via event_send
.
Alternatively an additional WebSocket endpoint just for RFID events would be better than polling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will rework that
src/rfid.cpp
Outdated
} | ||
|
||
void RfidTask::setup(){ | ||
if (config_rfid_enabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this require a reboot after RFID is enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the lifetime of the connection to the PN532 is fully handled in loop()
so this is kind of redundant. I better remove it.
src/rfid.cpp
Outdated
// Send to MQTT broker | ||
DynamicJsonDocument data{JSON_OBJECT_SIZE(1) + uid.length() + 1}; | ||
data["rfid"] = uid; | ||
mqtt_publish(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use event_send
to also send over the websocket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient for that purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, that is used for filling in the status when polled or reporting to EmonCMS, event_send
will send the value over MQTT and the WebSocket so any clients get updated immediately rather than up to 30 seconds later.
Thank you for the input, @jeremypoulter ! I will take a deeper look into everything and will continue this conversation when I have further questions. |
Now the PN532 driver and the RfidTask are separate modules. And I changed the RFID section in the dashboard so it doesn't require polling from the UI anymore. |
src/pn532.cpp
Outdated
|
||
if (!config_rfid_enabled()) { | ||
status = DeviceStatus::NOT_ACTIVE; | ||
return SCAN_FREQ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to poll if RFID is not enabled, can just return MicroTask.Infinate
and manually wake the task when the config is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Only for the PN532 driver or should I also do it so in the RfidTask
? And what would be the best way to detect when config_rfid_enabled()
turns on again? (another case as example is sufficient)
src/pn532.cpp
Outdated
|
||
listen = true; | ||
//"flush" ACK | ||
delay(30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't use delay
should set some state and drop out of loop
with a return of 30
and perform the action next time round, as it happens read
is the first thing that is done in the loop so might be ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will change it
Also can you merge in the changes from |
The other thing that needs doing is adding a build option to enable the RFID code as it needs to be disabled on the ESP32 Gateway board. In the default config the I2C conflicts with the Ethernet and despite the large expansion connector there are not suitable alternative pins for I2C so it is best to remove. |
Okay, will merge from Do you have an opinion about to what extend the build option should skip the RFID code? My ideas are:
Edit: Option 1 seems to have the the least potential for errors. A possible improvement would be to surround everything in |
I have implemented your feedback, except to stop polling when |
Thanks will check it over. As a preference I would prefer the code is compiled out rather than Dummy/Null drivers. Will make for a smaller binary. You can leave the infinite return for another PR if you would prefer. There is a function in app_config that notifies of changes to the config. Not an ideal way of doing it, but is ok. |
@matth-x I have done some basic testing and it looks mostly ok but there are still a few issues. I need this merged to unblock some other bits, so I am going to merge as with the latest changes it is mostly ok. Please raise tickets for the following and fix as separate PRs:
|
Any news on the OpenEVSE reader hardware @chris1howell? Thanks! |
The RFID integration supports two modes:
The
RfidTask
contains a custom driver for the NXP PN532 to fit best into the OpenEVSE architecture.The changes from the OCPP library include:
DBUG
, added log levelsTriggerMessage
support improved, addedClearCache
message type and more configuration keysChanges of the OCPP integration:
BootNotification
supportFurther changes are: