Skip to content

Conversation

drensber
Copy link
Contributor

The .iface_status method of the wifi_mgmt_ops API needs to be added so that the "wifi status" command on the network shell will work.

@github-actions github-actions bot added the area: Wi-Fi Wi-Fi label Apr 16, 2025
@drensber drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch 2 times, most recently from 246a979 to 40a8960 Compare April 16, 2025 15:15
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Minor nits about the coding style, you could see what the rest of the airoc_wifi.c looks like and follow suit.

@drensber drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch 2 times, most recently from beff44c to f09fb8d Compare April 17, 2025 13:28
@drensber
Copy link
Contributor Author

I changed all of these things according to your preferences. Looking at the Zephyr Coding Guidelines, I'm not sure that any of them are actually established rules, so perhaps that's why the automated checks didn't catch any of them.

@drensber
Copy link
Contributor Author

I think the twister failure is a problem on your side, not a problem with the code that I pushed. Can someone look into this and re-run the tests?

@rlubos
Copy link
Contributor

rlubos commented Apr 17, 2025

I think the twister failure is a problem on your side, not a problem with the code that I pushed. Can someone look into this and re-run the tests?

Looked like some environmental issue, I've retriggered failed stage, tests are now running.

@drensber drensber requested a review from jukkar April 17, 2025 15:35
jukkar
jukkar previously approved these changes Apr 19, 2025
@kartben kartben requested a review from Copilot April 19, 2025 07:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds an .iface_status method to the Infineon WiFi driver so that the "wifi status" command on the network shell works as expected.

  • Added the implementation of airoc_iface_status to report interface mode, state, SSID, channel, and other connection parameters.
  • Updated the wifi_mgmt_ops structure to include the new .iface_status method.
  • Introduced an additional header include for whd_wlioctl.h required by the new functionality.
Comments suppressed due to low confidence (1)

drivers/wifi/infineon/airoc_wifi.c:774

  • The variable 'airoc_if' is referenced without a visible declaration in this diff. Verify that it is properly declared and initialized in an accessible scope before use.
if (airoc_if == NULL) {

Comment on lines 806 to 858
strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);

Copy link

Copilot AI Apr 19, 2025

Choose a reason for hiding this comment

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

Using strncpy without explicitly appending a null terminator risks leaving status->ssid unterminated. Consider ensuring the string is null-terminated (and that the destination buffer is large enough) after copying.

Suggested change
strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);
if (status->ssid_len < sizeof(status->ssid)) {
strncpy(status->ssid, bss_info.SSID, status->ssid_len);
status->ssid[status->ssid_len] = '\0'; // Ensure null-termination
} else {
LOG_ERR("SSID length exceeds buffer size");
return -EINVAL; // Return an error if the buffer is too small
}

Copilot uses AI. Check for mistakes.

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 Copilot's advice is invalid in this particular case. The SSID is defined by zephyr to be a character string with a max length of 32 and an accompanying SSID_len field to specify the actual length. If I were to use one character of the ssid field as a null terminator, the maximum length would become 31.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to NULL terminate SSID buffer in the status, but you are using source length to copy to destination, so, this can overflow, it's always better to use destination length or do explicit NULL checking.

@drensber drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch from f09fb8d to 298f0a3 Compare April 24, 2025 16:06
@drensber
Copy link
Contributor Author

drensber commented May 6, 2025

Do I need to do anything else here, or are we just waiting for someone else to review?

@beechwoods-software
Copy link

@Kludentwo @rlubos @krish2718 @MaochenWang1 Could someone else please review this? My understanding is that after a certain amount of time (60 days?) it will just be closed to inactivity. I don't think this should be too controversial of a PR. Even if it isn't a perfect implementation, it's better than having "wifi status" or .iface_status() not work at all, and any deficiencies can be addressed after there's a baseline implementation in place.

Copy link
Contributor

@krish2718 krish2718 left a comment

Choose a reason for hiding this comment

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

Please remove all whitespace changes or move them to a separate commit for easier review.

Comment on lines 806 to 858
strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to NULL terminate SSID buffer in the status, but you are using source length to copy to destination, so, this can overflow, it's always better to use destination length or do explicit NULL checking.

@drensber drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch from d1d7176 to ce817c7 Compare June 5, 2025 17:36
The .iface_status method of the wifi_mgmt_ops API needs to be added
so that the "wifi status" command on the network shell will work.

Signed-off-by: Dave Rensberger <[email protected]>
@drensber drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch from ce817c7 to 2699472 Compare June 5, 2025 17:54
Copy link

sonarqubecloud bot commented Jun 5, 2025

@drensber
Copy link
Contributor Author

drensber commented Jun 5, 2025

Can someone please restart the automated tests? They failed to even start for some reason "The job was not acquired by Runner of type hosted even after multiple attempts".

@drensber
Copy link
Contributor Author

drensber commented Jun 6, 2025

@jukkar Do you have the ability to restart the automated tests? They timed out when I did my latest push for some reason.

Copy link

github-actions bot commented Aug 6, 2025

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 6, 2025
@drensber
Copy link
Contributor Author

drensber commented Aug 6, 2025

@jukkar How do I get the "Stale" label removed? I'd like to see this PR accepted. As far as I know, it's just waiting for further review.

@github-actions github-actions bot removed the Stale label Aug 7, 2025
@drensber
Copy link
Contributor Author

@krish2718 @jukkar Again... What do I need to do to get this merged? As far as I'm aware, it's just waiting on review. Is there a way I can at least get the "Stale" tag removed so that it doesn't automatically close?

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants