-
Notifications
You must be signed in to change notification settings - Fork 11
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
Comments
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. and code:
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... |
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? |
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 :) |
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:
What do you think? |
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? |
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... |
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
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!
The text was updated successfully, but these errors were encountered: