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

Jump to bottom

Create and populate UART message to log serial port information #17360

Closed
wants to merge 7 commits into from
Closed

Create and populate UART message to log serial port information #17360

wants to merge 7 commits into from

Conversation

peterbarker
Copy link
Contributor

No description provided.

peterbarker
peterbarker commented May 5, 2021
View reviewed changes
libraries/AP_HAL/UARTDriver.cpp Outdated

void AP_HAL::UARTDriver::update_logging_all()
{
hal.uartA->update_logging();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we might now have an array for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a lot nicer if we did...

Found in AP_SerialManager for the init:

    // initialise serial ports    
    for (uint8_t i=1; i<SERIALMANAGER_NUM_PORTS; i++) {    
        auto *uart = hal.serial(i);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a lot nicer if we did...

Found in AP_SerialManager for the init:

    // initialise serial ports    
    for (uint8_t i=1; i<SERIALMANAGER_NUM_PORTS; i++) {    
        auto *uart = hal.serial(i);

Thanks.

Interestingly different from other places using num_serial in place of SERIALMANAGER_NUM_PORTS

libraries/AP_HAL/utility/RingBuffer.h Outdated
Comment on lines 586 to 606
typedef ObjectArray<float> FloatArray; No newline at end of file
typedef ObjectArray<float> FloatArray;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this

@peterbarker peterbarker added the WIP label May 5, 2021
@peterbarker peterbarker force-pushed the pr/logger-MAV-uart-stats branch from 21bfd73 to b9bf32b Compare May 11, 2021 07:56
khancyr
khancyr reviewed May 11, 2021
View reviewed changes
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

not done for linux ?

andyp1per
andyp1per reviewed May 11, 2021
View reviewed changes
libraries/AP_HAL/UARTDriver.cpp Outdated
_stats.tx_buffer_min = -1; // using integer wrap
}

const struct log_UART pkt = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we log throughput in baud somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably just the sum of bytes transmitted.

@tridge tridge removed the DevCallEU label May 12, 2021
@tridge
Copy link
Contributor

tridge commented May 12, 2021

nice idea! just needs a few less 'stuff' commits :-)

WickedShell
WickedShell reviewed May 12, 2021
View reviewed changes
libraries/AP_HAL/UARTDriver.cpp Outdated
instance : (uint8_t)_instance,
rxq_min : stats_copy.rx_buffer_min,
rxq_max : stats_copy.rx_buffer_max,
rxq_avg : (stats_copy.rx_buffer_bytes_available_count == 0) ? (uint16_t)0 : uint16_t(stats_copy.rx_buffer_bytes_available_sum/stats_copy.rx_buffer_bytes_available_count),
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a style thing, but casting the whole return from the touple to uint16_t is a bit less verbose

libraries/AP_HAL/UARTDriver.cpp Outdated
// reset statistics
memset(&_stats, 0, sizeof(_stats));
_stats.rx_buffer_min = -1; // using integer wrap
_stats.tx_buffer_min = -1; // using integer wrap
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you have to reset the max's to 0 as well?

libraries/AP_HAL/UARTDriver.h

// logging-related variables
struct Stats {
HAL_Semaphore sem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the semaphore within the struct? This means we are copying the semaphore for as part of copying the struct when we are logging (IE it's just wasted time on copying it)

libraries/AP_HAL_ChibiOS/UARTDriver.cpp
@@ -660,6 +661,7 @@ bool UARTDriver::discard_input()
}

_readbuf.clear();
update_readbuf_stats();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to have the semaphore here?

libraries/AP_HAL_ChibiOS/UARTDriver.cpp
}
void UARTDriver::update_readbuf_stats()
{
const uint32_t available = _readbuf.available();
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like it's easier to add the semaphore holding in here, rather then making all the callers be responsible for having it.

libraries/AP_HAL_ChibiOS/UARTDriver.cpp
{
WITH_SEMAPHORE(_stats.sem);
_stats.rx_bytes += count;
update_readbuf_stats();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have update_readbuf_stats take the number of new read bytes as an argument and adding it internally? It seems like you always basically have a new number to add (or 0 I guess for the clear case)

libraries/AP_HAL_ChibiOS/UARTDriver.cpp Outdated
@@ -706,6 +716,14 @@ int16_t UARTDriver::read()
update_rts_line();
}

#if HAL_LOG_UART_STATS
{
WITH_SEMAPHORE(_stats.sem);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be curious how much time grabbing this semaphore for all the drivers that read byte by byte add's up to? I'm just thinking about the companion computer/MAVLink links at near 1 megabaud. It's probably fine, it just seems like it could be a bit of a factor.

@peterbarker peterbarker force-pushed the pr/logger-MAV-uart-stats branch from b9bf32b to 49f9b9f Compare July 12, 2021 02:27
@peterbarker peterbarker force-pushed the pr/logger-MAV-uart-stats branch from 49f9b9f to 440318a Compare July 30, 2023 07:54
@IamPete1 IamPete1 mentioned this pull request Mar 18, 2024
Closed
@peterbarker peterbarker force-pushed the pr/logger-MAV-uart-stats branch from 440318a to f94c335 Compare March 25, 2024 23:36
@IamPete1 IamPete1 mentioned this pull request Apr 6, 2024
Merged
@peterbarker peterbarker closed this Apr 9, 2024
@peterbarker peterbarker deleted the pr/logger-MAV-uart-stats branch April 9, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WickedShell WickedShell WickedShell left review comments

@khancyr khancyr khancyr left review comments

@andyp1per andyp1per andyp1per left review comments

Assignees
No one assigned
Labels
WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@peterbarker @tridge @WickedShell @khancyr @andyp1per