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

A number of comments / issues and proposed fixes #41

Open
Gyosa3 opened this issue May 27, 2023 · 6 comments
Open

A number of comments / issues and proposed fixes #41

Gyosa3 opened this issue May 27, 2023 · 6 comments

Comments

@Gyosa3
Copy link

Gyosa3 commented May 27, 2023

Hi and thanks so much for this code, this really spared me some headaches!

While trying to setup this platform I bumped into a few questions and issues, nothing important in view of the enormous service it gives, and I thought I'd create a list below of things that cross my mind (so I edit on the fly when I think of something else instead of creating one issue per idea):

  • this component is also available via HACS (!?), so I got it from there but it was totally outdated, That's confusing and I had to overwrite the files from the repo afterwards. That's a shame as the updates would be easier to follow for users if they were pushed via HACS. Custom updater is fine though, but HACS is well integrated with HA also to warn of new versions, etc. I'd suggest to adopt the tree structure required for HACS in your repo so that it works automatically. (I can work on it if that suits you)

  • there is a lack of documentation about what to do after you copy the files, I see some users fail to think about rebooting before adding the yaml config, and it fails. Maybe a doc update could be good. I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

  • I was confused by the explanation on 'route codes' vs. 'timing point codes', it was not clear and for me it looks the same. I tried your first explanation via the stop code, that is also called stop area code (!) and got a second code, never called route code, that looked like the timing point code, so I was already confused. If they are the same, why proposing 2 versions to configure the entity, and why give it 2 names? I think both versions of the sensor conf return the same data anyway. It all worked fine for me using timing point code and filtering out the line I need, while I see other users had issues with stop code and route code. I am not sure if it is still relevant to maintain both options? You can still propose 2 methods to retrieve the time point code, either via the stop code or via the line code, but at the end, only the timing stop code would be relevant to create the sensor entity. (I can work on it if that suits you)

  • I found an issue with stops that are end of line, the list of timings may retrieve both arrivals and departures of the same tram/bus if the timing point code is unique or the route code is unique. I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

  • I made a certain number of tests before I could end up with the entities that suit me, and because there is no uniqueID on the entities, they are not accessible in edit mode in HA so I cannot delete the older entities. One nice improvement would be to add a uniqueID to the entities so that you can edit/delete them in the UI. I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

  • sometimes the bus or tram is comming earlier than the scheduled time (?!) I was wondering if there is an interest to test for the sign of the delay, because it could be also negative, and therefore the '+' should become a '-' in the state. I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

  • My personal preference is to keep the original time and add "+1m' to indicate the delay, even for the first sensor. I have also changed this code for my own taste, but I have not shared it. That could be a config option, to either display the expected time or the original time + delay, to the choice of the user. I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

  • sometimes the delay is weird, like
    image
    this could be worth testing on the delay and discarding too high values (is this why the tram displays show 'oponthoud' when they have data out of bound?). I have proposed to fix this with pull request Many-updates-in-one pull request... #40.

Hope it helps, and thanks again for the great work!

@Gyosa3
Copy link
Author

Gyosa3 commented Jun 2, 2023

Hi

looking at the cases when the delay is 1440, I found out that this seems to happen when the transport is not announced late but early. When the expected time is 1 minute in advance it shows +1440m, when it is 2 minutes in advance it shows +1439m, etc.

Looking at raw data:
image

and code:

               calculate_delay = expected_arrival_time - target_departure_time
               delay = round(calculate_delay.seconds / 60)

It seems that the calculation must be done differently if the transport is in advance, the time difference does not work as expected when difference is negative...

@Paul-dH
Copy link
Owner

Paul-dH commented Jun 2, 2023

Hi @Gyosa3 Really appreciate your list of detailed findings. The reason that the development went a bit south is that we moved to another house and our automation platform (knx) is not ready yet to tune with HA.

I'm at work now, I'll dive in to your PR this evening if that's ok?

@Gyosa3
Copy link
Author

Gyosa3 commented Jun 2, 2023

No worries, I'm trying this morning to fix the issue of the "negative delay" and will try to add it to the PR before you get to it.

I'd like to have it fixed before I start working on a remote LCD display to show the next trams :)

@Gyosa3
Copy link
Author

Gyosa3 commented Jun 2, 2023

So I have added changes to the pull request today. I'll stop there and open another one if I have time to work on the rest:

  • I'd like to make the platform reloadable from the Developer Tools screen without restarting HA (I must have restarted it 50 times today...)

  • I'd like to rehook to HACS with the latest version

  • I also see your templated sensors, I think I understand that it's just to display nothing if the state is 'unknown' at the end of the day when it's the last train or bus. I think it can better be fixed in the code, The state could simply be '-' or '--:--' when there is no more schedule.

What do you think?

@Paul-dH
Copy link
Owner

Paul-dH commented Jun 2, 2023

Hi, I've checked all your code but I have no clue how I could test all your changes. I have no Home Assistant installation available at the moment. I'm also browsing the HACS docs a bit to check out the changes needed to fix the repo's correct listing you mentioned.

Are you on the Home Assistant Discord by any chance?

@Gyosa3
Copy link
Author

Gyosa3 commented Jun 2, 2023

No I'm not on Discord sorry, I only contribute to code on the occasion when I bump into an issue on my own install.

I have no test lab, I only do limited tests on my live setup, I can't go too far...

Out of the 3 points above, I have proposed a fix in the pull request for the 3rd one (unknown schedules) and I let you see for updating HACS.

For the reload service, I'm trying something blindly from another custom component, and if that doesn't work that'll be above my skills...

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

No branches or pull requests

2 participants