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.

Sign up for GitHub

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

uORB::DeviceNode allocate buffer on advertise #12802

Closed
wants to merge 2 commits into from
Closed

uORB::DeviceNode allocate buffer on advertise #12802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
498d52e
uORB::DeviceNode allocate buffer initially
dagar Aug 26, 2019
9f8b481
uORB_tests cleanup
dagar Aug 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 42 additions & 57 deletions src/modules/uORB/uORBDeviceNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,30 @@ uORB::DeviceNode::DeviceNode(const struct orb_metadata *meta, const uint8_t inst

uORB::DeviceNode::~DeviceNode()
{
if (_data != nullptr) {
delete[] _data;
}
delete[] _data;

CDev::unregister_driver_and_memory();
}

int
uORB::DeviceNode::init()
{
int ret = CDev::init();

if (ret != PX4_OK) {
return ret;
}

_data = new uint8_t[_meta->o_size * _queue_size];

// failed to allocate
if (nullptr == _data) {
return -ENOMEM;
}

return PX4_OK;
}

int
uORB::DeviceNode::open(cdev::file_t *filp)
{
Expand Down Expand Up @@ -169,7 +186,7 @@ uORB::DeviceNode::copy_locked(void *dst, unsigned &generation)
{
bool updated = false;

if ((dst != nullptr) && (_data != nullptr)) {
if (dst != nullptr) {

if (_generation > generation + _queue_size) {
// Reader is too far behind: some messages are lost
Expand Down Expand Up @@ -224,11 +241,6 @@ uORB::DeviceNode::copy_and_get_timestamp(void *dst, unsigned &generation)
ssize_t
uORB::DeviceNode::read(cdev::file_t *filp, char *buffer, size_t buflen)
{
/* if the object has not been written yet, return zero */
if (_data == nullptr) {
return 0;
}

/* if the caller's buffer is the wrong size, that's an error */
if (buflen != _meta->o_size) {
return -EIO;
Expand Down Expand Up @@ -256,42 +268,6 @@ uORB::DeviceNode::read(cdev::file_t *filp, char *buffer, size_t buflen)
ssize_t
uORB::DeviceNode::write(cdev::file_t *filp, const char *buffer, size_t buflen)
{
/*
* Writes are legal from interrupt context as long as the
* object has already been initialised from thread context.
*
* Writes outside interrupt context will allocate the object
* if it has not yet been allocated.
*
* Note that filp will usually be NULL.
*/
if (nullptr == _data) {

#ifdef __PX4_NUTTX

if (!up_interrupt_context()) {
#endif /* __PX4_NUTTX */

lock();

/* re-check size */
if (nullptr == _data) {
_data = new uint8_t[_meta->o_size * _queue_size];
}

unlock();

#ifdef __PX4_NUTTX
}

#endif /* __PX4_NUTTX */

/* failed or could not allocate */
if (nullptr == _data) {
return -ENOMEM;
}
}

/* If write size does not match, that is an error */
if (_meta->o_size != buflen) {
return -EIO;
Expand Down Expand Up @@ -379,10 +355,12 @@ uORB::DeviceNode::ioctl(cdev::file_t *filp, int cmd, unsigned long arg)
*(int *)arg = get_priority();
return PX4_OK;

case ORBIOCSETQUEUESIZE:
//no need for locking here, since this is used only during the advertisement call,
//and only one advertiser is allowed to open the DeviceNode at the same time.
return update_queue_size(arg);
case ORBIOCSETQUEUESIZE: {
ATOMIC_ENTER;
int ret = update_queue_size(arg);
ATOMIC_LEAVE;
return ret;
}

case ORBIOCGETINTERVAL:
if (sd->update_interval) {
Expand All @@ -394,6 +372,7 @@ uORB::DeviceNode::ioctl(cdev::file_t *filp, int cmd, unsigned long arg)

return OK;


case ORBIOCISPUBLISHED:
*(unsigned long *)arg = _published;

Expand Down Expand Up @@ -534,11 +513,6 @@ uORB::DeviceNode::poll_notify_one(px4_pollfd_struct_t *fds, pollevent_t events)
bool
uORB::DeviceNode::appears_updated(SubscriberData *sd)
{
// check if this topic has been published yet, if not bail out
if (_data == nullptr) {
return false;
}

// if subscriber has interval check time since last update
if (sd->update_interval != nullptr) {
if (hrt_elapsed_time(&sd->update_interval->last_update) < sd->update_interval->interval) {
Expand Down Expand Up @@ -618,7 +592,7 @@ int16_t uORB::DeviceNode::process_add_subscription(int32_t rateInHz)
// send the data to the remote entity.
uORBCommunicator::IChannel *ch = uORB::Manager::get_instance()->get_uorb_communicator();

if (_data != nullptr && ch != nullptr) { // _data will not be null if there is a publisher.
if (ch != nullptr) {
ch->send_message(_meta->o_name, _meta->o_size, _data);
}

Expand Down Expand Up @@ -661,12 +635,23 @@ int uORB::DeviceNode::update_queue_size(unsigned int queue_size)
return PX4_OK;
}

//queue size is limited to 255 for the single reason that we use uint8 to store it
if (_data || _queue_size > queue_size || queue_size > 255) {
if (_queue_size > queue_size || queue_size > 255) {
julianoes marked this conversation as resolved.
Show resolved Hide resolved
// queue size is limited to 255 for the single reason that we use uint8 to store it
return PX4_ERROR;
}

uint8_t *data = new uint8_t[_meta->o_size * queue_size];

if (data == nullptr) {
return PX4_ERROR;
}

// switch to new data
memcpy(data, _data, _meta->o_size * _queue_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increasing the queue size does not work: due to the generation % _queue_size logic, a subscriber could access invalid data after this update.
Better keep the existing logic until we move the queue size to the meta-data.

delete[] _data;
_data = data;
_queue_size = queue_size;

return PX4_OK;
}

Expand Down
4 changes: 3 additions & 1 deletion src/modules/uORB/uORBDeviceNode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ class uORB::DeviceNode : public cdev::CDev, public ListNode<uORB::DeviceNode *>
DeviceNode(DeviceNode &&) = delete;
DeviceNode &operator=(DeviceNode &&) = delete;

int init() override;

/**
* Method to create a subscriber instance and return the struct
* pointing to the subscriber as a file pointer.
Expand Down Expand Up @@ -267,7 +269,7 @@ class uORB::DeviceNode : public cdev::CDev, public ListNode<uORB::DeviceNode *>
const uint8_t _instance; /**< orb multi instance identifier */
uint8_t *_data{nullptr}; /**< allocated object buffer */
hrt_abstime _last_update{0}; /**< time the object was last updated */
volatile unsigned _generation{0}; /**< object generation count */
unsigned _generation{0}; /**< object generation count */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this atomic? It's accessed concurrently w/o locks.

List<uORB::SubscriptionCallback *> _callbacks;
uint8_t _priority; /**< priority of the topic */
bool _published{false}; /**< has ever data been published */
Expand Down
Loading