- 
                Notifications
    
You must be signed in to change notification settings  - Fork 8.2k
 
wifi: esp32: enhance handling of AP connect events #75786
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
wifi: esp32: enhance handling of AP connect events #75786
Conversation
| 
           hi @ndrs-pst thanks for the contribution! Would you mind re-write the commit message, so it is more clear what was intended to achieve here?  | 
    
| 
           Yes, for sure. So if you can guide the direction of the message it would be great.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just whitespace noise, please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback about the whitespace noise. I've already removed it from the commit.
By the way, could you let me know when or in what situations I can remove such whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if it was intentional, then you can move that to a separate commit with commit logs detailing the reason. The code review is done per-commit esp. for large PR, so, thumb rule is one logical change per-commit.
          
 Ok, so perhaps, instead of naming the function calls used (which is visible in the diff), you could write about actual improvement that change brings to the esp32 wifi management.  | 
    
7d330cc    to
    1e535cf      
    Compare
  
    
        68453eb
      
    1e535cf    to
    68453eb      
    Compare
  
    | 
           Sorry for the inconvenience. I just realized that the link mode assignment was wrong. 
 There is no 802.11a, which is WIFI_2. 🫡  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another way would be to just memset the sta_info and only fill in non-default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that most of the data is non-zero except for twt_capable, memset may not be the choice in this situation. 😅
68453eb    to
    5658f22      
    Compare
  
    | 
           Ping @sylvioalves  | 
    
| 
           @sylvioalves Please take a look :)  | 
    
| 
           @ndrs-pst sorry it took me so long to check this, I just missed (a few times). LGTM. Would you mind taking the clang-format suggestions regarding comments and code style?  | 
    
This commit aims to improve the integration of `esp_wifi_drv` by providing the link mode information to `wifi_mgmt` when a station device is connected to the AP. Signed-off-by: Pisit Sawangvonganan <[email protected]>
5658f22    to
    d62ccb0      
    Compare
  
    
          
 Sure, I've already addressed it. :)  | 
    
| 
           Ping @sylvioalves 👀  | 
    
This commit aims to improve the integration of
esp_wifi_drvbyproviding the link mode information to
wifi_mgmtwhen a station deviceis connected to the AP.