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

SerialManager IOMCU fixups #29475

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Mar 9, 2025

Fix-ups for #29094.

The IOMCU now shows up as a full serial port with parameters (CubeOrangePlus), so the param docs should be added so the user knows what 50 is. This also adds defines for the default baud and protocol so the params don't show up as changed by the user.

image

I'm not sure if the intention was that the params should show up at all on boards other than CubeRed?

There is also a now unused stats tracker that can be removed now the IOMCU is in the main serial array.

@IamPete1 IamPete1 requested a review from bugobliterator March 9, 2025 20:34
@@ -184,7 +184,7 @@ const AP_Param::GroupInfo AP_SerialManager::var_info[] = {
// @DisplayName: Telem1 protocol selection
// @Description: Control what protocol to use on the Telem1 port. Note that the Frsky options require external converter hardware. See the wiki for details.
// @SortValues: AlphabeticalZeroAtTop
// @Values: -1:None, 1:MAVLink1, 2:MAVLink2, 3:Frsky D, 4:Frsky SPort, 5:GPS, 7:Alexmos Gimbal Serial, 8:Gimbal, 9:Rangefinder, 10:FrSky SPort Passthrough (OpenTX), 11:Lidar360, 13:Beacon, 14:Volz servo out, 15:SBus servo out, 16:ESC Telemetry, 17:Devo Telemetry, 18:OpticalFlow, 19:RobotisServo, 20:NMEA Output, 21:WindVane, 22:SLCAN, 23:RCIN, 24:EFI Serial, 25:LTM, 26:RunCam, 27:HottTelem, 28:Scripting, 29:Crossfire VTX, 30:Generator, 31:Winch, 32:MSP, 33:DJI FPV, 34:AirSpeed, 35:ADSB, 36:AHRS, 37:SmartAudio, 38:FETtecOneWire, 39:Torqeedo, 40:AIS, 41:CoDevESC, 42:DisplayPort, 43:MAVLink High Latency, 44:IRC Tramp, 45:DDS XRCE, 46:IMUDATA, 48:PPP, 49:i-BUS Telemetry
// @Values: -1:None, 1:MAVLink1, 2:MAVLink2, 3:Frsky D, 4:Frsky SPort, 5:GPS, 7:Alexmos Gimbal Serial, 8:Gimbal, 9:Rangefinder, 10:FrSky SPort Passthrough (OpenTX), 11:Lidar360, 13:Beacon, 14:Volz servo out, 15:SBus servo out, 16:ESC Telemetry, 17:Devo Telemetry, 18:OpticalFlow, 19:RobotisServo, 20:NMEA Output, 21:WindVane, 22:SLCAN, 23:RCIN, 24:EFI Serial, 25:LTM, 26:RunCam, 27:HottTelem, 28:Scripting, 29:Crossfire VTX, 30:Generator, 31:Winch, 32:MSP, 33:DJI FPV, 34:AirSpeed, 35:ADSB, 36:AHRS, 37:SmartAudio, 38:FETtecOneWire, 39:Torqeedo, 40:AIS, 41:CoDevESC, 42:DisplayPort, 43:MAVLink High Latency, 44:IRC Tramp, 45:DDS XRCE, 46:IMUDATA, 48:PPP, 49:i-BUS Telemetry, 50: IOMCU
Copy link
Contributor

Choose a reason for hiding this comment

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

serial manager does not control the IOMCU uart, so we should not have the parameter exposed. If the parameter is exposed it should do something

Copy link
Member

Choose a reason for hiding this comment

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

fixed here #29493

Copy link
Collaborator

Choose a reason for hiding this comment

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

so is this going to be removed? if not, put a WIKI NEEDED label on the PR please

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the param is still exposed on CubeRed we need the value in the description.

@tridge
Copy link
Contributor

tridge commented Mar 11, 2025

@bugobliterator can you please check? did you deliberately make SERIAL7 appear in param list for CubeOrangePlus and other IOMCU boards? That is a bad user experience showing parameters that do nothing

@bugobliterator
Copy link
Member

@bugobliterator can you please check? did you deliberately make SERIAL7 appear in param list for CubeOrangePlus and other IOMCU boards? That is a bad user experience showing parameters that do nothing

@tridge yes that unintentional. Here is a fix that should solve this issue. #29493

Copy link
Member

@bugobliterator bugobliterator left a comment

Choose a reason for hiding this comment

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

the stats changes are correct, except that all the rest of the changes will break CubeRed workflow.

The current behaviour is that the serial protocol is set to IOMCU for internal use only, it shouldn't have been exposed for other boards, that's oversight on my part. I have a fix for that here #29493

For cubered this parameter is defaulted to PPP but if BRD_IO_ENABLE is set its changed to IO Protocol in AP_Vehicle, that is the desired behaviour. And the serial is visible in parameters as well, for users to modify as well.

@@ -184,7 +184,7 @@ const AP_Param::GroupInfo AP_SerialManager::var_info[] = {
// @DisplayName: Telem1 protocol selection
// @Description: Control what protocol to use on the Telem1 port. Note that the Frsky options require external converter hardware. See the wiki for details.
// @SortValues: AlphabeticalZeroAtTop
// @Values: -1:None, 1:MAVLink1, 2:MAVLink2, 3:Frsky D, 4:Frsky SPort, 5:GPS, 7:Alexmos Gimbal Serial, 8:Gimbal, 9:Rangefinder, 10:FrSky SPort Passthrough (OpenTX), 11:Lidar360, 13:Beacon, 14:Volz servo out, 15:SBus servo out, 16:ESC Telemetry, 17:Devo Telemetry, 18:OpticalFlow, 19:RobotisServo, 20:NMEA Output, 21:WindVane, 22:SLCAN, 23:RCIN, 24:EFI Serial, 25:LTM, 26:RunCam, 27:HottTelem, 28:Scripting, 29:Crossfire VTX, 30:Generator, 31:Winch, 32:MSP, 33:DJI FPV, 34:AirSpeed, 35:ADSB, 36:AHRS, 37:SmartAudio, 38:FETtecOneWire, 39:Torqeedo, 40:AIS, 41:CoDevESC, 42:DisplayPort, 43:MAVLink High Latency, 44:IRC Tramp, 45:DDS XRCE, 46:IMUDATA, 48:PPP, 49:i-BUS Telemetry
// @Values: -1:None, 1:MAVLink1, 2:MAVLink2, 3:Frsky D, 4:Frsky SPort, 5:GPS, 7:Alexmos Gimbal Serial, 8:Gimbal, 9:Rangefinder, 10:FrSky SPort Passthrough (OpenTX), 11:Lidar360, 13:Beacon, 14:Volz servo out, 15:SBus servo out, 16:ESC Telemetry, 17:Devo Telemetry, 18:OpticalFlow, 19:RobotisServo, 20:NMEA Output, 21:WindVane, 22:SLCAN, 23:RCIN, 24:EFI Serial, 25:LTM, 26:RunCam, 27:HottTelem, 28:Scripting, 29:Crossfire VTX, 30:Generator, 31:Winch, 32:MSP, 33:DJI FPV, 34:AirSpeed, 35:ADSB, 36:AHRS, 37:SmartAudio, 38:FETtecOneWire, 39:Torqeedo, 40:AIS, 41:CoDevESC, 42:DisplayPort, 43:MAVLink High Latency, 44:IRC Tramp, 45:DDS XRCE, 46:IMUDATA, 48:PPP, 49:i-BUS Telemetry, 50: IOMCU
Copy link
Member

Choose a reason for hiding this comment

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

fixed here #29493

@IamPete1
Copy link
Member Author

@bugobliterator What is the plan with the serial port params on CubeRed? Are the only valid values IOMCU or PPP? In which case I don't think you need to expose the params, the switch can be done on only board IO enable param? If the params are exposed setting board IO enable should set and default the parameters. My goal with adding the defaults is to make sure that parameters that the user did not change don't show up as not default values in the log.

@bugobliterator
Copy link
Member

@bugobliterator What is the plan with the serial port params on CubeRed? Are the only valid values IOMCU or PPP? In which case I don't think you need to expose the params, the switch can be done on only board IO enable param? If the params are exposed setting board IO enable should set and default the parameters. My goal with adding the defaults is to make sure that parameters that the user did not change don't show up as not default values in the log.

Actually for CubeRed primary secondary comms can be DDS and mavlink as well, nothing is set in stone there, and would like to keep all the options open there. As far as the change to not show up as non-default value, I guess we can change the default value of the parameter instead of just setting the value itself.

@IamPete1 IamPete1 force-pushed the IOMCU_UART_fixups branch from 6faa4e4 to aad9115 Compare March 11, 2025 22:18
@IamPete1
Copy link
Member Author

IamPete1 commented Mar 11, 2025

@bugobliterator Thanks for the explination. I have added what I think is missing from #29493 and removed everything else.

The only thing that remains is there is no way for the hardware report WebTool to know which logged UART instance is the IOMCU. I had it as a special case of 100 but that was removed in #29094. For all the other UARTs I can lookup the params based on the index to find the protocol. After #29493 that method won't work on the IOMCU anymore. We could hard coded it back to 100 for the instance which equals HAL_UART_IOMCU_IDX but that breaks the ability to look it up from the params on CubeRed. I guess we could work it out from the new HAL_HIDE_SERIALx_PARAMS and the existing HAL_HAVE_SERIALx. Any better ideas?

For reference this is the what the tool used to plot:
image

@bugobliterator
Copy link
Member

bugobliterator commented Mar 12, 2025

@bugobliterator Thanks for the explination. I have added what I think is missing from #29493 and removed everything else.

The only thing that remains is there is no way for the hardware report WebTool to know which logged UART instance is the IOMCU. I had it as a special case of 100 but that was removed in #29094. For all the other UARTs I can lookup the params based on the index to find the protocol. After #29493 that method won't work on the IOMCU anymore. We could hard coded it back to 100 for the instance which equals HAL_UART_IOMCU_IDX but that breaks the ability to look it up from the params on CubeRed. I guess we could work it out from the new HAL_HIDE_SERIALx_PARAMS and the existing HAL_HAVE_SERIALx. Any better ideas?

@IamPete1 I think we can simply add protocol field to uart_info.txt. Where it can show which protocol its currently set to if available. That would be more informative long-term as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants