KoljaWindeler / ics

Integration that displays the next event of an ics link (support reoccuring events)
50 stars 13 forks source link

Wrong Date & failure(-1) #7

Closed ghost closed 4 years ago

ghost commented 4 years ago

Hi Kolja,

I have two problems that already were discussed here but are present in my installation.

First I am on Home Assistant 0.108.6 and ICS 3c93c1d

  1. I have a Holiday calendar which I use due to the corona time to find out if it is a holiday day, a workday or a weekend day.

This is my ICS file: https://outlook.office365.com/owa/calendar/xxx/xxx/calendar.ics

The problem is that e.g. today there is an event called URLAUB from today till tomorrow. The plugin only sees the next event on the 27th.

With the version before there were no problem-

  1. I have an Events calendar and there I have the failure(-1) problem. I have double checked the ics file and I am not able to find an error.

That's the events ICS file: https://outlook.office365.com/owa/calendar/xxx/xxx/calendar.ics

Thank you

Greets Daniel

EDIT: Anonymized the ICS links as the issues are solved!

KoljaWindeler commented 4 years ago

Hi, i can reproduce the both errors and will have a look shortly

KoljaWindeler commented 4 years ago

Hi Daniel, thanks for sharing the links, that helps a lot during the debugging: Regarding the first link: the ICS file shows 20200224 - 20200226 20200306 - 20200307 20200320 - 20200321 20200323 - 20200328 20200406 - 20200408 20200414 - 20200415 20200420 - 20200422 20200427 - 20200429

Since all but the last events have passed .. i would expect the last to be shown .. and thats the 27th .. so I'd say everything works fine there ..

The second issue a really tricky. One of your events has a timezone aware start, but not a timezone aware re-occurrence rule .. thats what throws the exception. I'm using the recurring_ical_events package and that can't handle this combination. I'll look further into it, but that's a tricky one

EDIT: sunshine live Classics has a timezone aware start, timezone aware end but the re-occurrance is not timezone aware... could you have a look if you can change that?

ghost commented 4 years ago

Hi Kolja,

OK, if I change the events to separate events it works (EDIT: but only the event tomorrow, not today).

In the previous version of your plugin (which I am using at the moment to handle the date problem) I have an event with remaining 0. That's what I expect and what I am using for automations. Is there a chance to get that behavior back?

The second thing: I completely deleted the sunshine live Classics event but I still have failure(-1). Both with the previous and the actual version.

Thank you

Greets Daniel

KoljaWindeler commented 4 years ago

Hi, regarding 1: Yes that's something I've "updated" so events that were DATES (no time) will now have a time assigned to it (0:00:00) .. that was required to compare DATES with DATETIMES (with time). otherwise one couldn't use mixed calendars. So once the time is over (which it was for you at 0:00:00) the calendar will jump to the next event. The question is: when would you like to load the next event? one day later? what about events that have e.g. start 14:00 end 14:30, when would you load the next event? at the moment this is happening right at 14:00 ..

I could imagine to add something like an "extend" setting, so the start is delayed by that amount, e.g. 24h but that is a very confusing setting, don't you think? Would it be possible for you to change your automation?

Kolja

second: I'm still checking .. won't be that easy

ghost commented 4 years ago

Hmm, OK ... I see. Not easy.

My problem is that I need a sensor which tells me that NOW is something, you understand?

I have an entity which can have three states: Arbeit, Frei & Urlaub.

With this state I am controlling the heating or the time in which my espresso machine is powered on (to get the cups warm).

So for me it's not as necessary to know how much days are remaining till the next event but more necessary to know if the event is today or not.

For your garbage thing this behavior is OK: I also have a blinking button which is blinking one day before the "Gelber Sack" event because I have to put it out one day before the event is.

Greets Daniel

KoljaWindeler commented 4 years ago

I see .. I've added another option called show_ongoing which will be False per default. If set to True it will .. well .. show ongoing events .. I've tested this with your calendar and get the intended behavior.

This update will be a bigger one, because it will add config_flow and configure / change the addon without reboots of home assistant. I'll need some beta testers, would you like to test before I'll add it to github? You'd have to remove it via HACS and download zip file to your custom_components directory ..

ghost commented 4 years ago

Hi, cool. Yes, of course I will test it for you!

KoljaWindeler commented 4 years ago

great have a try: https://drive.google.com/file/d/1HTundn5PeqU0EDJPznsJfFJR8PcCoYKd/view?usp=sharing

This version adds config flow, adds you show_ongoing and checks every single event of the calendar for timezone before forwarding it to unroll the re-occurance .. meaning: it should work witz your problem nr 2. Please check if the timing is correct (might be of due to my modification of timezones)

ALL comments are welcome.

PS: There is a hidden folder .translation in the zip, make sure to copy this as well.

ghost commented 4 years ago

Hi Kolja,

as what I can see both issues are fixed with the beta. I tested some things with force_update on and for me it works fine.

I will do more testing an report if I find new issues. But at the moment it looks fine.

Thank you!

Greets Daniel

KoljaWindeler commented 4 years ago

perfect .. I've done some polishing to the code that shouldn't break the behavior and pushed it to github. Thanks for your input! Kolja

ghost commented 4 years ago

One thing but no error:

If you want to use the show_ongoing attribute and want to determine if on that day is an event and this event started for example yesterday you have to use <= 0 instead of == 0 in the automation. It's also pretty useful when you for example want to know how long an event is going on.

ghost commented 4 years ago

OK, with the newest version the plugin completely runs amok:

I am getting a -2 day state with my holidays which were from Monday till Tuesday and reoccurs next monday and tuesday.

I am getting a 1 day state for an event which is not tomorrow.

Very weird at the moment.

Here the two new ICS files: ics_files.zip

This is my config:

- platform: ics
    name: "Gelber Sack"
    url: https://outlook.office365.com/owa/calendar/xxx/xxx/calendar.ics
    id: 1
  - platform: ics
    name: "Urlaub"
    url: https://outlook.office365.com/owa/calendar/xxx/xxx/calendar.ics
    id: 2
    startswith: URLAUB
    force_update: 60
    show_ongoing: true
  - platform: ics
    name: "Events"
    url: https://outlook.office365.com/owa/calendar/xxx/xxx/calendar.ics
    id: 3
    show_ongoing: true
    force_update: 60
    startswith: Unicorne
KoljaWindeler commented 4 years ago

damn :) ... i can reproduce both ... a little background: I'm opening the calendar file with the icalendar package, scan all events and change them from date to datetime, to make them compareable. after that i feed the data to the recurring_ical_events package to unroll all reoccuring events. after that i sort them and take the first event that matches the filter.

So 1) I really don't know why recurring_ical_events thinks that your urlaub is still ongoing but i can fix that by checking that NOW must be smaller then START or END (if show_ongoing is set). I've added this .. that works .. will update soon, although it scares me a bit that the error happened in the first place. ...

2) same here, your RRULE says every thursday BUT [list of dates] .. i guess recurring_ical_events isn't parsing the BUT rule correctly. I guess I'll have to raise a bug at the recurring_ical_events package for that. otherwise I would have to do the processing myself .. and that seams pointless. I have the slight suspicion that my conversion to datetime might(!) cause this .. i'll check

KoljaWindeler commented 4 years ago

TRL: new version, please try and let me know

Long story: Ok .. i think I've figured it out .. the root cause of everything are ics events like this:

BEGIN:VEVENT
RRULE:FREQ=YEARLY;UNTIL=20200524T220000Z;INTERVAL=1;BYMONTHDAY=25;BYMONTH=5
SUMMARY:Towel Day
DTSTART;VALUE=DATE:20200525
DTEND;VALUE=DATE:20200526
DTSTAMP:20200422T142528Z
END:VEVENT

That is an event with datetime.date as start and end, but datetime.datetime (with time) as UNTIL. recurring_ical_events really didn't like that, and crashed (that was your inital finding).

I've "fixed" it by changing all DTSTART and DTEND to datetime.datetime before passing it to recurring_ical_events. The problem occures now with your events with EXDATE (exclude date) rule. that didn't match the rule anymore, because i've previously translated everything to datetime.datetime.

Sou, what I'm doing now is: once the calendar file is loaded:

  1. fix the RRULE so it matches the DTSTART https://github.com/KoljaWindeler/ics/blob/c25af36af9a0767a5728e25bfe916b71917ccca3/custom_components/isc/sensor.py#L153
  2. let recurring_ical_events process all events with RRULE and EXDATE
  3. convert everything to datetime.datetime https://github.com/KoljaWindeler/ics/blob/c25af36af9a0767a5728e25bfe916b71917ccca3/custom_components/isc/sensor.py#L162
  4. sort it, so the next date is the first https://github.com/KoljaWindeler/ics/blob/c25af36af9a0767a5728e25bfe916b71917ccca3/custom_components/isc/sensor.py#L169

... and further down the line make sure that the event is either still ahead, or show_ongoing is set and we're still ahead of the end date https://github.com/KoljaWindeler/ics/blob/c25af36af9a0767a5728e25bfe916b71917ccca3/custom_components/isc/sensor.py#L200

My only concern is still that I have to check for start / end > now() .. I'll do some more diggin

... let me know Kolja

PS: you can see in the log of home assistant how many RRULEs were fixed, for that you have to activate logging on debug level:

logger:
  default: info
  logs:
    custom_components.ics: debug
KoljaWindeler commented 4 years ago

ok, I can explain the last uncertainty as well .. your URLAUB that started 20200420 ends 20200422. My understanding is that it actually starts 20200420 at 00:00:00 and ends 20200422 at 00:00:00. recurring_ical_events says that it ends 20200422 at 23:59:59

thats why the extra filter step was needed. kolja

ghost commented 4 years ago

Ah, OK ... I have similar problems in my SQL Queries at work sometimes.

I will do some testing. At the moment I get the right states.

ghost commented 4 years ago

Hi Kolja,

I found another problem which might be easy to solve this time (I hope).

If there is no next event the entity has the state "no next event (-1)". Unfortunately the remaining attribute is also -1 which is the same like when the event started yesterday and is ongoing.

To determine the exact state I am using "<= 0" to get the right "type of day" (work, weekend, holiday) so that I am getting the wrong type of day at the moment.

Do you have an idea how to handle that?

Greets Daniel

EDIT: OK, i got this done with a failure switch in Node-RED where I am searching for "no next event (-1)" in the state.

KoljaWindeler commented 4 years ago

ok, the question remains if "none" or "null" would be better. I could imagine that this would lead to other errors due to the none working type compatibility, right? I'll try some testing .. I guess -9999 could work as well? Kolja

ghost commented 4 years ago

-9999 would not solve the problem for me right now. I don't know how other users are using your plugin.

"none" or "null" would be good for me but I don't know which problems this would make.

My solution in Node-RED is working at the moment so I don't know how important a solution on your side is at the moment.

KoljaWindeler commented 4 years ago

Ok, this is really tricky

{{ None < 1 }} is actually true, {{ Null < 1 }} returns an error, {{ "no update" < 1 }} is false, but {{ "no update" > 1 }} is obviously true ..

I'll change it to -999 in the future just because it's likelier that -1 really happens compared to -999, easier to check and typesafe

Kolja