ALFN Decode & Add to API#504
Conversation
| if (bc == 15) | ||
| pids_complete_fm(&st->pids); |
There was a problem hiding this comment.
The decode stage having to tell the PIDS decoder when a frame is complete seems a bit messy. Do you think it might make sense to instead pass in the mode (FM/AM) as an argument to pids_frame_push so that the PIDS decoder can take care of this check?
There was a problem hiding this comment.
I could do that. That means I will need to check for bc == 15 or bc == 6 in pids. I was trying to reduce that.
There was a problem hiding this comment.
Providing the mode as an argument to pids_frame_push does mean I have to do this:
if (mode == NRSC5_MODE_FM && bc == 15)
pids_complete_fm(st);
if (mode == NRSC5_MODE_AM && bc == 8)
pids_complete_am(st);
There was a problem hiding this comment.
I guess there's an extra mode check if you put it inside, but it does seem better from an encapsulation perspective.
There was a problem hiding this comment.
Yeah.. I guess so. Whatever makes the code less messy especially long term.
There was a problem hiding this comment.
Would this cause issues with Advanced services modes where there's many PIDS frames?
There was a problem hiding this comment.
Good question. To be honest, I haven't read up on them to know exactly how they work. But we should probably design things in such a way that we can deal with the increased PIDS bandwidth and presence of LLDS PDUs in the future.
It could be that the current design where the decode layer informs the PIDS layer about the end of a frame works better.
| { | ||
| if (st->alfn[st->alfn_frame].received == 0xFF) | ||
| { | ||
| if (st->alfn_frame == 3 && st->alfn_upper_idx == -1) |
There was a problem hiding this comment.
Is there a reason for this section of code to be inside the received == 0xFF check, given that the find_mod4 stuff only cares about the two least significant bits?
There was a problem hiding this comment.
Omg, you are right. This is from old iterations of the code. You are correct that theres no reason for this to be inside received == 0xFF anymore.
There was a problem hiding this comment.
Also, is there a reason we're only attempting to find the upper index when st->alfn_frame == 3?
There was a problem hiding this comment.
Well, there's no reason to try to find ALFN upper if 3 frames haven't been received yet. Technically this could be considered unnecessarily when 3 frames have received already.
There was a problem hiding this comment.
Lastly, the st->alfn_upper_idx == -1 check worries me slightly, since it means that if we make the wrong decision, the decoder is poisoned forever. I suppose the likelihood of that is low though, since at least two of the four SIS PDUs involved would need to be corrupted for that to happen, and the chance of a single SIS PDU passing the CRC12 check is 1/4096.
There was a problem hiding this comment.
Yeah, I can understand that. An idea is to reset alfn_upper_idx if an upper frame has been received incomplete or I could just repeatedly look for the upper frame
There was a problem hiding this comment.
Well, there's no reason to try to find ALFN upper if 3 frames haven't been received yet.
Sure, but what if the SNR is low, and PDUs are getting dropped due to invalid CRCs? We'll only check once every four frames which PDUs from the four most recent frames were received, when we'd have a better chance by considering any four consecutive frames.
There was a problem hiding this comment.
This code only runs once per frame, so I'm not at all concerned about efficiency in terms of CPU utilization.
There was a problem hiding this comment.
We'll only check once every four frames which PDUs
Yeah, that's true. Possibly removing this change would improve decoding ALFN on AM a lot.
There was a problem hiding this comment.
For that reason, it may be sensible to simplify a bit and get rid of some of the ifs.
|
One last thing that may be worth consideration: If we fail to decode the ALFN value for a given frame, but we haven't lost sync, we could consider incrementing the most recent ALFN and returning that, since we know that ALFNs are sequential. I'm not convinced this in necessarily worth doing, but it could be that clients would prefer to see an ALFN event for every L1 frame instead of having gaps. If we went this route, I suppose it would also bring up the question of whether we should report ALFN events for L1 frames received before the first valid ALFN is decoded (with some sort of dummy value to indicate that the ALFN is unknown). |
|
I was thinking something similar but for something else. I think I see a point there from what your saying. Since ALFN can be used to identify the frame, it wouldn't be super useful if sometimes the ALFN isn't reported |
|
I can kind of see that being able to identify a frame may be useful for debugging |
ALFN (Absolute L1 Frame Number) is what is required after
leap_seconds_offsetto calculate the current UTC timestamp from the station (#453 ).ALFN is sent through PIDS logical channel and different for AM and FM. AM was a pain in the butt. I was questioning my sanity.
UTC offset from
local_timeon the other hand is basically always incorrect.Sample code is included in the CLIs to calculate the UTC time withStations are just completely messed up. It looks like current time is being pushed out as UTC on most transmitters.gmtime.Some reasons why to incorporate this silly number other than the terrible GPS timing: