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

ITC-3 uses incorrect timestamp for traffic counting #454

Open
otterdahl opened this issue Sep 26, 2024 · 24 comments
Open

ITC-3 uses incorrect timestamp for traffic counting #454

otterdahl opened this issue Sep 26, 2024 · 24 comments
Labels
equipment Bug in equipment ITC-3

Comments

@otterdahl
Copy link
Contributor

In recent runs, ITC-3 reports an incorrect time stamp 1970-01-01T00:00:00.000Z for start of traffic counting (S0201-S0208)

Example

    "sS": [
        {
            "sCI": "S0207",
            "n": "start",
            "q": "recent",
            "s": "1970-01-01T00:00:00.000Z"
        }
]
@otterdahl
Copy link
Contributor Author

The validator doesn't catch this issue. Perhaps the test case can be extended

@otterdahl otterdahl added validator Bug in the validator equipment Bug in equipment labels Sep 26, 2024
@emiltin
Copy link
Contributor

emiltin commented Sep 26, 2024

The schema for S0207 should catch this, as the 'n' field in a sS 'start' element must be a standard rsmp timestamp:
https://github.com/rsmp-nordic/rsmp_schema/blob/main/schemas/tlc/1.2.1/statuses/S0201.json#L51

But it seems this is not working for some reason.

@SwarcoPalm
Copy link

I think the timestamp format is valid enough, I'm guessing Davids suggestion is that this test controller indeed has not been counting since 1970?

@emiltin
Copy link
Contributor

emiltin commented Jan 3, 2025

ok, so the schema is correct, but the device is reporting on incorrect datetime

@emiltin emiltin removed the validator Bug in the validator label Jan 3, 2025
@sveitech
Copy link

sveitech commented Jan 6, 2025

The RSMP program logs a timestamp whenever a detection is made. If there are no detections, then it defaults to the 1970 timestamp.

What timestamp do you propose to put in, when there haven't been made any detections yet?

@emiltin
Copy link
Contributor

emiltin commented Jan 7, 2025

The RSMP program logs a timestamp whenever a detection is made

The "start" attribute is defined as "Time stamp for start of measuring", not when the first detection was made.

Start of measurement is when the last update was send. I think that should cover all cases, since you cannot get a status update without first subscribing, and when you subscribe you first get a status update.

@sveitech
Copy link

sveitech commented Jan 7, 2025

That makes sense. So, in every case, the timestamp just reflects when RSMP transmits a status update, either as a response to a status request or a subscription.

@emiltin
Copy link
Contributor

emiltin commented Jan 7, 2025

Yes. I think it could be clearer in the docs. @otterdahl?

@otterdahl
Copy link
Contributor Author

otterdahl commented Jan 9, 2025 via email

@emiltin
Copy link
Contributor

emiltin commented Jan 9, 2025

I wasn't aware of this section 4. I find some of the requirements in section 4 somewhat surprising, or at least very specific in the sense that they affect the delivery mechanism defined in the core spec, e.g. don't send an initial status update, send only on even intervals.

It might be helpful to add a link to the section 4 in the section of each status message related to traffic data? E.g. in:
https://rsmp-nordic.github.io/rsmp_sxl_traffic_lights/1.2.1/sxl_traffic_light_controller.html#s0201

@otterdahl
Copy link
Contributor Author

It might be helpful to add a link to the section 4 in the section of each status message related to traffic data? E.g. in: https://rsmp-nordic.github.io/rsmp_sxl_traffic_lights/1.2.1/sxl_traffic_light_controller.html#s0201

Sure. I've opened a PR: rsmp-nordic/rsmp_schema#124

@sveitech
Copy link

Would it be possible to design 201-208 in a way that does not violate the RSMP Core specification? I.e. 201-208 requires that no initial update is sent, vs. the Core spec stating that an immediate update message should be sent upon subscription.

Our RSMP implementation has never supported this violation, and we have heard no complaints about this so far, for the more than 10 years the implementation has existed.

In my opinion, it is dangerous to have such violations of the core spec. It is confusing both for implementers and consumers, and it makes it more difficult to validate that the core spec is adhered to. Especially in this case, where the documentation was not correct.

So I would like to discuss if we can remove this "violation". Is there any danger to sending out the initial data? In my opinion it is the right thing to do.

For example, let's say that there has been no queries or subscriptions, up until now. RSMP has detected 102 vehicles since the daemon was started.

Now a subscription is initiated, for lets say every 5 minutes. Upon subscription, the RSMP daemon will immediately send a StatusUpdate message back, containing the 102 vehicles detected so far, with a timestamp equal to "now" (just as if a normal query was performed). From then on, StatusUpdates will proceed as normal, containing only data since last interval. This all sounds "correct" to me.

You even have this statement in the documentation (https://rsmp-nordic.github.io/rsmp_core/3.1.5/applicability/basic_structure.html#status-messages):

The reason for sending the response immediately is because subscriptions usually are established shortly after RSMP connection establishment and the supervision system needs to update with the current statuses and events.

@otterdahl
Copy link
Contributor Author

So I would like to discuss if we can remove this "violation". Is there any danger to sending out the initial data? In my opinion it is the right thing to do.

The reason why we added this exception was due to a potential edge case right when the subscription starts. As far as I understand, traffic counting data is only truly useful if it all detectors start measuring at the same time and measures during even time intervals, (e.g. 15:00:00- 15:05:00). If this is not the case than limit what kind of analysis we can perform when we compare traffic data between detectors/controllers. We were concerned that if the send an initial StatusUpdate upon subscription start, then we might include partial or too much data (e.g. 15:00:00-15:03:23 or 14:23:23-15:00:00).

But as you mention, this is a edge case that only applies to the start of subscription and after that it really no problem. I guess that the supervision system might as well filter out any initial data where the measuring period exceeds or deviates from the expectation.

So if this exception causes issues then I think we can consider it's removal.

@sveitech
Copy link

I think I have found a special case, that would require some clarification:

Say that a subscription has been set up for S0201. Every 5 minutes., starting from 15:00

Now, a get request is issued to S0201 at 15:02. What data should this get-request return?

Should it return the counts from 15:00 to 15:02 and contain a starttime=15:00? What then happens to the subscription that has been set up? Should it still return the data from 15:00 to 15:05? Or should it be reset to return from 15:02 to 15:05?

@otterdahl
Copy link
Contributor Author

otterdahl commented Jan 17, 2025 via email

@sveitech
Copy link

Nice. Thank you for the clarification. So it sounds like get-requests during a subscription cannot "reset" or interfere with the subscription. I.e. it is merely peeking. What happens to get-requests when there is no subscription? Should they also never reset the count?

@otterdahl
Copy link
Contributor Author

otterdahl commented Jan 17, 2025 via email

@emiltin
Copy link
Contributor

emiltin commented Jan 20, 2025

I think is an important core-spec discussion, opened rsmp-nordic/rsmp_core#213.

@emiltin
Copy link
Contributor

emiltin commented Jan 20, 2025

I'm also in favour of removing any requirement in the SXL that override core specs.

@emiltin
Copy link
Contributor

emiltin commented Jan 20, 2025

Based on dicussions above, I think very old timestamps (1970) could be flagged as an error in the validator?

@sveitech
Copy link

sveitech commented Feb 4, 2025

Another issue I found regarding S0201-S0208:

What happens if you only subscribe partially to an object? Say for example that you only subscribed to the "vehicles" object in S0201, and not "starttime" (unlikely, I know)?

This is just to prove another point, that it is bad that some parts of the SXL have special requirements and violate the core spec. All these special cases listed here: https://rsmp-nordic.github.io/rsmp_sxl_traffic_lights/1.0.15/traffic_data.html

requires that you subscribe to the entire SXL object, and not just sub-parts of it. I think that this should be stated clearly on the link above, and that partial subscriptions will not be supported, or at least, lead to undefined behavior.

Alternatively, it should be stated that subscribing to sub-parts of an object in S0201-S0208, enables the "traffic data" special cases, for all sub-objects, i.e. their values may change even though you did not subscribe to them.

Also I think it would be good to disable "subscribe-on-change" for these objects. I.e. you cannot make both an interval subscription and subscribe-on-change.

@emiltin
Copy link
Contributor

emiltin commented Feb 5, 2025

Attributes that must always to subscribe to together could perhaps be send as a single json object:

{
  "n": "data",
  "s": { "vehicles": 45, "starttime": "2019-09-26T12:48:00.285Z" }
}

@sveitech
Copy link

sveitech commented Feb 5, 2025

This is already the case. Elements can be subscribed to as a single json message, and will be replied back as a single message, if they have the same subscription interval.

My point is, there is nothing forcing a user to do this. You could legally set up a subscription for "vehicles" alone, for example. But it is currently undefined what then happens to the "starttime" field, as there is no subscription to it.

@sveitech
Copy link

sveitech commented Feb 5, 2025

As you may have noticed by now, I have been making a lot of posts about S0201-S0208 specifically, to point out all the discrepancies, inconsitencies and undefined parts.

This is to prove a point that the current specification in https://rsmp-nordic.github.io/rsmp_sxl_traffic_lights/1.0.15/traffic_data.html is insufficient.

Perhaps it should be boiled down to its most basic ideas, and then leave the rest of the edge cases as "undefined behavior". For example, the page could just state:

  • S0201-S0208 are meant for interval subscription only (no on-change).
  • "clean" time-points are used (as the example shows in the link)
  • values are only reset on event emision.

ALL other cases are undefined, and should not be used (i.e. Do not query, do not change subscription interval while subscription is running, e.t.c.).

I think this would make it more clear what the intent of these objects are, and much easier to implement both on the server and in the clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
equipment Bug in equipment ITC-3
Projects
None yet
Development

No branches or pull requests

4 participants