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

feat: add optional "reduced rate subscription" functionality #9

Open
t-bre opened this issue Feb 9, 2023 · 5 comments · May be fixed by #16
Open

feat: add optional "reduced rate subscription" functionality #9

t-bre opened this issue Feb 9, 2023 · 5 comments · May be fixed by #16
Assignees
Labels
feature New feature or request
Milestone

Comments

@t-bre
Copy link
Member

t-bre commented Feb 9, 2023

Description

Add the feature of a "reduced rate subscription" in which notifications are only made to the subscriber if a certain time period has passed since the last arrival of the given CAN message. This may be useful if a node wants to listen to a fast rate broadcast message but not have to fully process it too often.

Additional Notes

Potential application is on-car telemetry.

@t-bre t-bre added the feature New feature or request label Feb 9, 2023
@t-bre
Copy link
Member Author

t-bre commented Feb 9, 2023

@rureverek I know you've done some work on this concept yourself for on-car telemetry with the table of last received timestamps. I thought it might actually be a useful feature to integrate into RTCAN itself for other projects to use as well. If you'd like to work on this, assign yourself the issue and let me know. Otherwise, I can do it or find someone else who wants to work on it.

@rureverek
Copy link

@t-bre I can take it. My idea for implementation is as follows:

  • add additional entry in rtcan_hashmap_node_t for last timestamp
  • create additional parameter in rtcan_subscribe() function - the reduced rate and add the functionality in the body of the function

Is that make sense?
I wondering how to implement it to add reduced rate as the optional argument. I assume there should exist smart macro that does such thing (similarly like printf function)?
Like here? https://stackoverflow.com/questions/27795767/optional-arguments-in-c-function

@t-bre
Copy link
Member Author

t-bre commented Feb 12, 2023

Sounds right, rtcan_hashmap_node_t will need to store the last timestamp for sure. I would also add some kind of "subscription rate" value to each subscriber (rtcan_subscriber_t) which represents the number of ticks after which a message should be delivered again. So the relevant RX thread function would change to something like:

rtcan_hashmap_node_t* node_ptr = find_hashmap_node(rtcan_h,
                                                   msg_ptr->identifier);

if (node_ptr != NULL)
{
    // check change in time since last reception of this CAN message
    UINT time_now = tx_time_get();
    UINT time_delta = time_now - node_ptr->last_rx_timestamp;

    // update last receive timestamp
    node_ptr->last_rx_timestamp = time_now;

    rtcan_subscriber_t* subscriber_ptr = node_ptr->first_subscriber_ptr;

    while (subscriber_ptr != NULL)
    {
        // only deliver message if enough ticks have passed
        if (time_delta >= subscriber_ptr->subscription_rate)
        {
            msg_ptr->reference_count++;
            
            tx_status = tx_queue_send(subscriber_ptr->queue_ptr,
                                      &msg_ptr,
                                      TX_NO_WAIT);
    
            if (tx_status != TX_SUCCESS)
            {
                msg_ptr->reference_count--;
            }
        }

        subscriber_ptr = subscriber_ptr->next_subscriber_ptr;
    }

This way, the "subscription rate" argument can simply be set to zero for subscribers that want to receive all of the incoming messages. You could do something with variadic functions which I think is what you're suggesting, but that's something that I generally find to be unsafe (and complicated!). Also if you go by MISRA C guidelines (which I try to follow as much as possible) it actually has a rule that explicitly requires stdarg.h not to be used. I think just having an argument that can be set to zero is safer / easier. Could perhaps define a constant named something like RTCAN_RX_RATE_MAX or RTCAN_RX_ALL to make it more clear what the argument means in code that doesn't use this function.

@t-bre t-bre added this to the STAG 9 milestone Feb 12, 2023
@rureverek
Copy link

@t-bre I think we need to change a bit the concept and last_timestamp should be stored in rtcan_subscriber_t.
The problem is in this line:

    // update last receive timestamp
    node_ptr->last_rx_timestamp = time_now;

where you cannot update timestamp before transmit because it will stuck and never deliver any message. E.g if the can transmit every 2ms and the subscription rate is 10ms, the last_rx_timestamp will be updated every 2ms and time_delta will never reach 10ms threshold. That's why it should be implemented on subsciber's side, something like this:

            rtcan_hashmap_node_t* node_ptr = find_hashmap_node(rtcan_h,
                                                               msg_ptr->identifier);

            if (node_ptr != NULL)
            {             
                rtcan_subscriber_t* subscriber_ptr = node_ptr->first_subscriber_ptr;

                while (subscriber_ptr != NULL)
                {
                    UINT time_now = tx_time_get();
                    UINT time_delta = time_now - subscriber_ptr->last_timestamp;

                    // only deliver message if enough ticks have passed
                    if(time_delta >= subscriber_ptr->subscription_period)
                    {
                        msg_ptr->reference_count++;
                        
                        tx_status = tx_queue_send(subscriber_ptr->queue_ptr,
                                                &msg_ptr,
                                                TX_NO_WAIT);

                        if (tx_status != TX_SUCCESS)
                        {
                            msg_ptr->reference_count--;
                        }
                        // update last transmit timestamp
                        subscriber_ptr->last_timestamp = time_now;
                    }

                    subscriber_ptr = subscriber_ptr->next_subscriber_ptr;
                }

As long as one subscriber correspond to one CAN_ID it should work well.

@t-bre
Copy link
Member Author

t-bre commented Feb 13, 2023

You're absolutely right, it should be a "last delivered timestamp" in each subscriber. So only update the timestamp if it reaches the part where the message is sent to the queue as you've done.

I wouldn't call it a "transmit timestamp" in the comment there, it's a bit confusing since we're on the receiving end (though another node IS transmitting that message). I'd call it "last delivered timestamp" or something.

@rureverek rureverek linked a pull request Feb 13, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants