PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.28k stars 13.43k forks source link

Downsampling problem for multiple subscribers in uORB. #16951

Open Donny9 opened 3 years ago

Donny9 commented 3 years ago

Describe the bug

I find that in the current code I can't get really downsampled topic data for topics whose queue isn't 1 and has more than one subscriber by uORB API.

The following example: A topic has one publisher(name is P) whose queue is 10 to publish data in freq:100HZ, and it has three subscriber(name are A, B and C), they subscribe at 100Hz , 50Hz, 30HZ respectively, they correspond to intervals of 10000us , 20000us 33333 us respectively.

Use the last 10 data publish to illustrate for correct downsampled case:

             ^10ms
  P -> data1   data2   data3   data4   data5   data6   data7   data8   data9   data10  pub in 100hz
  A <- data1   data2   data3   data4   data5   data6   data7   data8   data9   data10  sub in 100hz 
             ^20ms
  B <- data1           data3           data5           data7           data9           sub in 50hz
             ^33ms
  C <- data1                        data4                        data7                 sub in 30hz

Theoretically, subscribers with different frequencies get sample data which should be the sample data at the corresponding time point.

But the current code obtains the sample value from the queue in order after satisfying the sampling interval.

Use the last 10 data publish to illustrate for master:

             ^10ms
  P -> data1   data2   data3   data4   data5   data6   data7   data8   data9   data10  pub in 100hz
  A <- data1   data2   data3   data4   data5   data6   data7   data8   data9   data10  sub in 100hz 
             ^20ms
  B <- data1           data2           data3           data4           data5           sub in 50hz
             ^33ms
  C <- data1                        data2                        data3                 sub in 30hz

so that's wrong, the corresponding code is:

uORB::DeviceNode::copy(void *dst, unsigned &generation)
{
...
            ATOMIC_ENTER;
            const unsigned current_generation = _generation.load();

            if (current_generation == generation) {
                /* The subscriber already read the latest message, but nothing new was published yet.
                * Return the previous message
                */
                --generation;
            }

            // Compatible with normal and overflow conditions
            if (!is_in_range(current_generation - _queue_size, generation, current_generation - 1)) {
                // Reader is too far behind: some messages are lost
                generation = current_generation - _queue_size;
            }

            memcpy(dst, _data + (_meta->o_size * (generation % _queue_size)), _meta->o_size);
            ATOMIC_LEAVE;

            ++generation;

            return true;
...
}

To solve this problem, my current measure is to add up the generation intervals according to the multiples of the highest sampling frequency and the current subscription frequency.

However, for the relationship between the highest sampling frequency and the current subscription frequency is not divisible, the error between the time point and the sampling value will become larger and larger.

So, Do you have good suggestion for this problem?

Donny9 commented 3 years ago

@davids5 @dagar What do you think?

dagar commented 3 years ago

I think I understand what you're saying. I was vaguely aware of the imperfect interaction between subscriptions with intervals (eg uORB::SubscriptionInterval) and queued topics, but didn't necessarily think it would be a real problem.

For various reasons I've wanted to push the publication timestamp into uORB itself so that it's set atomically with the actual data copy. If we had the timestamp for each msg within uORB::DeviceNode then we'd be able to do this perfectly for queued topics and subscriptions with intervals right?

ronenlam commented 4 months ago

Hi @dagar @Donny9 , Any news w.r.t to this problem?

If I understand the problem described correctly than for example:

If this is the case than this uORB::SubscriptionInterval_ is not at all doing what it should do and should not be used

Thanks in advance Ronen L.