-
Notifications
You must be signed in to change notification settings - Fork 30
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
Send data or alerts to Telegram Messenger chat #442
base: master
Are you sure you want to change the base?
Conversation
Small changes to userdefines stylecheck fixes stylecheck fix 2
f96a2d9
to
7b4abdc
Compare
Hmm, isn't Telegram a closed / commercial service? The client seems free, but the server? Is there only one hosted by that company? Somehow I feel supporting Matrix (Messenger: element) would be more appropriate as they also have FOSS server. |
While Telegram has not the best of reputation with human communities, it also was one of the very first to push for an easy to use chatbot system and API. Therefore it was the easiest to implement - but also the only relatively common messenger I found at all with an Arduino library... |
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.
some comments.
commenting looked strange on github (github bug), always showing me duplicate forms. strange.
hopefully this doesn't mess up everything.
@@ -37,7 +37,7 @@ | |||
|
|||
// DIP switches | |||
static Switches switches; | |||
|
|||
float accumulated_Count_Rate = 0.0, accumulated_Dose_Rate = 0.0; |
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.
this is a bit dirty considering i just got rid of most of the global variables mess.
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.
Agreed. But it would be equally bad style to redo the whole dose calculation with local storage of the previous state from "publish" again in "transmit". I think it might be time to merge all the activities with dose & count stuff in a single function...? Maybe we should indeed meet e.g. at ShackSpace and refactor first on the whiteboard and then directly in code?
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.
s. #444
|
||
static unsigned int transmitCounter = 0; | ||
transmitCounter++; | ||
if (transmitCounter == 27720) transmitCounter = 0; // 27720 % 1..12 == 0 |
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 sure i understand...
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.
Aim was for the first numbers (i.e. 1 to 12) to avoid an hiccup (i.e. change in interval length) when the counter rolls over. As the actual time the sending is happening is slowly shifting anyway, it probably doesn't matter...
Can be resolved with a better timer handling, to avoid the dependency on the initial 2.5 min for sensor.community
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.
i still don't understand. and you need to add the comment to the source code, not here on github or in a year nobody will remember why we have that.
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 adapt to fixed interval after #444
iotwebconf::CheckboxParameter sendLocalAlarmToMessengerParam = iotwebconf::CheckboxParameter("Send local alarm via Messenger", "sendLocalAlarmToMessenger", sendLocalAlarmToMessenger_c, CHECKBOX_LEN, sendLocalAlarmToMessenger); | ||
iotwebconf::IntTParameter<int16_t> sendDataToMessengerEveryParam = | ||
iotwebconf::Builder<iotwebconf::IntTParameter<int16_t>>("sendDataToMessengerEvery"). | ||
label("Send data via Messenger every N x 2.5min\n(0=never,24=1/h,576=1/d,4032=1/week,max:27719)"). |
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.
hardcoding this would need that we remember to change this place if we ever change the 2.5min elsewhere.
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.
Agreed. We need a better architecture for data processing handling... s. above
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.
you can also write here "every N measurement intervals" or so and that solves this problem.
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.
with that, one can't give specific values for 1/h, 1/d, 1/w though.
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.
s. above
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.
some more comments.
considering the added size for the telegram bot lib, does the code still fit onto the older devices with small flash chip?
transmit_data(tubes[TUBE_TYPE].type, tubes[TUBE_TYPE].nbr, dt, hv_pulses, counts, current_cpm, | ||
transmit_data(tubes[TUBE_TYPE].type, tubes[TUBE_TYPE].nbr, | ||
dt, hv_pulses, counts, current_cpm, |
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.
was this beyond some line length limit or why did you reformat this?
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.
maybe wordwrap from my editor? will check
@@ -69,9 +71,22 @@ void setup_transmission(const char *version, char *ssid, bool loraHardware) { | |||
c_customsrv.wc->setCACert(ca_certs); | |||
c_customsrv.hc = new HTTPClient; | |||
|
|||
if ((strlen(telegramBotToken) < 40) || (strlen(telegramChatId) < 7)) | |||
sendDataToMessengerEvery = -1; |
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.
maybe "Interval" instead of "Every"?
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.
s. above
void transmit_userinfo(String tube_type, int tube_nbr, float tube_factor, unsigned int cpm, unsigned int accu_cpm, float accu_rate, | ||
int have_thp, float temperature, float humidity, float pressure, int wifi_status, bool alarm_status) { |
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.
indenting seems not adjusted. int directly below String?
// if we don't have a valid Messenger config, do not send to Messenger | ||
if (sendDataToMessengerEvery < 0) | ||
return; |
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.
either remove the empty line 322 or add another empty line after 325 to get consistent?
if (((sendDataToMessengerEvery > 0) && (transmitCounter % sendDataToMessengerEvery == 0)) | ||
|| ((sendDataToMessengerEvery >= 0) && alarm_status)) { |
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.
this seems to be a triple use variable / in band signalling and that's bad:
- it is the interval, if > 0
- it means only send when alarm is on when 0
- it means disabled when < 0
can we have different variables to make it better readable / configurable?
e.g. could be one for the interval and another one for the mode (disabled, always, only on alarm).
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.
that would maybe also solve the use case "send in intervals of N" but "only when alarm is on".
sprintf(message, "<b>--- MULTIGEIGER ALERT ! ---</b>\n<code>%s</code> rate too high:\n%.2f nSv/h (accumulated: %.2f nSv/h)", localEspId, cpm*tube_factor*1000/60, accu_rate*1000); | ||
else | ||
sprintf(message, "<b>--- MULTIGEIGER ALERT ! ---</b>\n<code>%s</code> rate too high:\n%d (accumulated: %d)", localEspId, cpm, accu_cpm); |
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.
still todo.
also: the unit of measurement is missing for the lower case. "GM counts"?
if (tube_nbr > 0) | ||
sprintf(message, "MultiGeiger <code>%s</code> rates:\n%.2f nSv/h (accumulated: %.2f nSv/h)%s", localEspId, cpm*tube_factor*1000/60, accu_rate*1000, have_thp ? thp_text : ""); | ||
else | ||
sprintf(message, "MultiGeiger <code>%s</code> CPM:\n%d (accumulated: %d)%s", localEspId, cpm, accu_cpm, have_thp ? thp_text : ""); |
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.
same here.
// To communicate with the Telegram Messenger on your phone you need to create a bot. | ||
// Starting point: https://core.telegram.org/bots | ||
// You will get a Bot token, please provide this via Web Config. | ||
// Form: "XXXXXXXXXX:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" | ||
// Important: Start your bot channel with the command "/start" and receive the Chat ID. | ||
// In order to get MultiGeiger messages to your specific chat, please provide this Chat ID via Web Config. | ||
// Form: "123456789" |
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 this be either in the form the user will see or in the documentation?
iotwebconf::CheckboxParameter sendLocalAlarmToMessengerParam = iotwebconf::CheckboxParameter("Send local alarm via Messenger", "sendLocalAlarmToMessenger", sendLocalAlarmToMessenger_c, CHECKBOX_LEN, sendLocalAlarmToMessenger); | ||
iotwebconf::IntTParameter<int16_t> sendDataToMessengerEveryParam = | ||
iotwebconf::Builder<iotwebconf::IntTParameter<int16_t>>("sendDataToMessengerEvery"). | ||
label("Send data via Messenger every N x 2.5min\n(0=never,24=1/h,576=1/d,4032=1/week,max:27719)"). |
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.
with that, one can't give specific values for 1/h, 1/d, 1/w though.
@@ -31,3 +31,4 @@ lib_deps= | |||
IotWebConf@^3.1.0 | |||
MCCI LoRaWAN LMIC library | |||
h2zero/NimBLE-Arduino | |||
witnessmenow/UniversalTelegramBot@^1.3.0 |
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 it required to fix it to that version?
Hi @ThomasWaldmann, I started a separate PR to better handle different events while processing GMC data. Hopefully this will help address some of the issues above. |
First commit sending regular data and/or alerts to Telegram messenger on phone